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/08 11:55:07, Massi wrote:
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...
Ok.
https://codereview.chromium.org/17568015/diff/8001/src/bounds-check-elimination.cc#newcode265
src/bounds-check-elimination.cc:265: ZoneList<Element> elements_;
I think you could get away with a plain-old C array for these elements,
since it is only allocated once and never grows.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-bounds-check-removal.cc
File src/hydrogen-bounds-check-removal.cc (right):
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-bounds-check-removal.cc#newcode101
src/hydrogen-bounds-check-removal.cc:101: */
It is still not clear to me what order the code visits the blocks, and
how exactly that is accomplished. Will need in-person discussion for
this.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-bounds-check-removal.cc#newcode180
src/hydrogen-bounds-check-removal.cc:180: &failure, unsafe);
It might actually be clearer to inline this PerformStep method and then
refactor its pieces into different configurations.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-bounds-check-removal.cc#newcode293
src/hydrogen-bounds-check-removal.cc:293: void
CollectInductionVariableData(HBasicBlock* bb);
It's OK IMO to go ahead and pull up these other methods into the class
definition.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc#newcode1986
src/hydrogen-instructions.cc:1986: result->base = value;
Thanks for the comment; now that I understand the patterns you would
like to match, I don't think you even need a recursive method. You can
first just try to match the outer |/& operation, and if it succeeds,
then try to match the inner +/- on the non-constant side, and if it
fails, try to match the +/- on the whole thing. It would be far more
clear than a loop that does not loop.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc#newcode2241
src/hydrogen-instructions.cc:2241: bool
InductionVariableData::TokenCanRepresentLoopGuard(Token::Value token) {
With the name change I see now more clearly what you meant. I actually
think you want to compute whether a given {InductionVariable, Token,
limit} is false leading out of a loop, since that is what is interesting
below?
In other words, I am not sure if breaking this operation out into its
own method is worthwhile.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc#newcode2256
src/hydrogen-instructions.cc:2256: HBasicBlock* out_branch) {
This is kind of a strange name for this method. I think it should be
inlined (with a comment) into the second of the two methods which I
suggest making the below method into.
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc#newcode2269
src/hydrogen-instructions.cc:2269: HInstruction* end =
predecessor->last();
This method could benefit from being split up into one that matches the
branching pattern from which we derive information (namely the given
block is entered from a predecessor that has a branch on an induction
variable) and a method that consumes that branching pattern and updates
either the induction variable data or the additional limit.
You could have a little datastructure that represents basically the
triple
{InductionVariableData induction_variable, Token token, Instruction
limit).
https://codereview.chromium.org/17568015/diff/21001/src/hydrogen-instructions.cc#newcode2288
src/hydrogen-instructions.cc:2288: if (is_else_branch) {
Please just negate the token above, and don't use an intermediate
is_else_branch boolean variable.
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.
