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.
On 2013/07/02 13:45:06, titzer wrote:
Please name this file with a "hydrogen-" prefix.
"hydrogen-bounds-check-elimination.cc" might be unwieldy, so perhaps
"hydrogen-bce.cc" will be OK.
hydrogen-bounds-check-removal.cc is just as long as other names...
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode35
src/bounds-check-elimination.cc:35: class Element {
On 2013/07/02 13:45:06, titzer wrote:
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.
Added comments.
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode39
src/bounds-check-elimination.cc:39: HBasicBlock* block() { return
block_; }
On 2013/07/02 13:45:06, titzer wrote:
Why do you have accessors for all this state in the table? Why is any
other
class using the internals?
Removed unneeded accessors.
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode107
src/bounds-check-elimination.cc:107: Element()
On 2013/07/02 13:45:06, titzer wrote:
At the very least the constructor should accept the block as a
parameter.
I don't think I can do it because this is the element of an array, so I
need a parameterless constructor.
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) {
On 2013/07/02 13:45:06, titzer wrote:
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.
Added a comment.
I still think that having the loop is simpler because to implement it
recursively I'd have to pass along "has_offset" and "current" as
additional arguments, which would mean having an "internal" recursive
version of the method or something similar.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2093
src/hydrogen-instructions.cc:2093:
added_constant()->InsertBefore(constant_insertion_point);
On 2013/07/02 13:45:06, titzer wrote:
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.
I tried that, with the ternary ?:, but added_index() and
first_check_in_block() have different types, therefore writing the
ternary expression would have required casts... I just did not find the
expression with those casts readable at all :-(
But yes, I can invoke "InsertBefore" in each if branch :-)
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2101
src/hydrogen-instructions.cc:2101: new_index->ClearAllSideEffects();
On 2013/07/02 13:45:06, titzer wrote:
What side effects might the new index have that you are clearing? Some
weird
JS-iness?
IIRC calling "toNumber()", which in our case is guaranteed not to
happen.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2103
src/hydrogen-instructions.cc:2103: added_index_ =
HBitwise::cast(new_index);
On 2013/07/02 13:45:06, titzer wrote:
There are several assignments of private variables directly and then
also getter
uses. Suggest just going with a direct private access for consistency.
The logic was to use the private variable for writing and the getter for
reading, I added a setter for writing.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2119
src/hydrogen-instructions.cc:2119: BitwiseDecompositionResult
decomposition;
On 2013/07/02 13:45:06, titzer wrote:
Probably better to initialize this struct at the declaration site
(either with a
constructor or explicitly here), rather than in the decompose method.
Added parameterless constructor, even if I still think that it is
InductionVariableData::DecomposeBitwise that produces the result
therefore it should be the one with the responsibility to initialize it.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2174
src/hydrogen-instructions.cc:2174: int32_t
InductionVariableData::ComputeIncrement(HPhi* phi,
On 2013/07/02 13:45:06, titzer wrote:
Comments documenting this method.
Done.
https://codereview.chromium.org/17568015/diff/8001/src/hydrogen-instructions.cc#newcode2180
src/hydrogen-instructions.cc:2180: if (operation->left() == phi) {
On 2013/07/02 13:45:06, titzer wrote:
You can further shorten these branches with &&.
Done.
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#newcode475
src/hydrogen.h:475: void NewJoke();
On 2013/07/02 13:45:06, titzer wrote:
Was ist das?
Debugging leftover :-)
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.
