https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc
File src/bounds-check-elimination.cc (right):
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode1
src/bounds-check-elimination.cc:1: // Copyright 2013 the V8 project
authors. All rights reserved.
Please name this file with a "hydrogen-" prefix.
"hydrogen-bounds-check-elimination.cc" might be unwieldy, so perhaps
"hydrogen-bce.cc" will be OK.
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode35
src/bounds-check-elimination.cc:35: class Element {
I find it hard to understand why this class serves both as a table of
induction variable information and also the state needed to iterate over
the blocks in the correct order. The dominator traversal here is unlike
others, e.g. the one in GVN, and I find that it obscures the underlying
algorithm here. And there is zero documentation.
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode39
src/bounds-check-elimination.cc:39: HBasicBlock* block() { return
block_; }
Why do you have accessors for all this state in the table? Why is any
other class using the internals?
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode107
src/bounds-check-elimination.cc:107: Element()
At the very least the constructor should accept the block as a
parameter.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode1986
src/hydrogen-instructions.cc:1986: while (true) {
I do not understand why this pattern matching is a loop. It can only
iterate from the ADD or SUB cases, both of which check that the masks
are both 0--but that seems redundant because all the other cases return.
Then you have a sentinel to indicate that you have matched an offset
with an ADD or SUB in order to force the OR case to abort.
Basically, I cannot figure which patterns this code matches or is
supposed to match. In most situations pattern matching is best done
recursively and I would advocate strongly to do so here. Also, a comment
would help a lot--including a grammar of which patterns would be
matched, for example, would save the reader a ton of work decoding this.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2093
src/hydrogen-instructions.cc:2093:
added_constant()->InsertBefore(constant_insertion_point);
Just move this code up into the if and save two lines. You will also
then have a redundant branch on added_index != NULL which you can then
fold together.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2101
src/hydrogen-instructions.cc:2101: new_index->ClearAllSideEffects();
What side effects might the new index have that you are clearing? Some
weird JS-iness?
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2103
src/hydrogen-instructions.cc:2103: added_index_ =
HBitwise::cast(new_index);
There are several assignments of private variables directly and then
also getter uses. Suggest just going with a direct private access for
consistency.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2119
src/hydrogen-instructions.cc:2119: BitwiseDecompositionResult
decomposition;
Probably better to initialize this struct at the declaration site
(either with a constructor or explicitly here), rather than in the
decompose method.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2174
src/hydrogen-instructions.cc:2174: int32_t
InductionVariableData::ComputeIncrement(HPhi* phi,
Comments documenting this method.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2180
src/hydrogen-instructions.cc:2180: if (operation->left() == phi) {
You can further shorten these branches with &&.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen.h#newcode470
src/hydrogen.h:470: void CollectInductionVariableData(HBasicBlock* bb,
I think you can move two of these methods to live in the
InductionVariableBlocksTable class and thus remove them from HGraph.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen.h#newcode475
src/hydrogen.h:475: void NewJoke();
Was ist das?
https://codereview.chromium.org/17568015/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
