https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc
File src/hydrogen-bounds-check-removal.cc (right):
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc#newcode48
src/hydrogen-bounds-check-removal.cc:48: * InitializeLoop() sets up the
table for a given loop.
These comments are probably better located on their respective methods.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc#newcode117
src/hydrogen-bounds-check-removal.cc:117: bool* unsafe) {
This is much more readable, good. I think we can do this iteratively
(and more cleanly) using an iterator. Essentially, the iterator would
yield one block at a time, and would have its own internal stack to keep
track of the DFS. As we discussed in person, I don't think that
intertwining the state needed to maintain the traversal within the
InductionVariableBlocksTable is readable, and I think the iterator is
the solution.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc#newcode208
src/hydrogen-bounds-check-removal.cc:208: bool
LoopPathsAreCheckedIteratively(bool* unsafe) {
When you convert this method to use an iterator, please don't pass in a
bool* for unsafe, but instead you can return one of 3 values.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc#newcode309
src/hydrogen-bounds-check-removal.cc:309: bool failure =
!LoopPathsAreChecked(&unsafe);
If this method just returns a value, then you don't need this weird
condition and pointer.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc#newcode1979
src/hydrogen-instructions.cc:1979: * (base - constant_offset) &
constant_and_mask
I don't understand why we don't match (base + constant) as well.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc#newcode2002
src/hydrogen-instructions.cc:2002: if
(bitwise->right()->IsInteger32Constant()) {
As per previous comments, please don't use a pointer here, when you can
change the order of comparisons and get rid of it.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc#newcode2251
src/hydrogen-instructions.cc:2251: if (result >= MAX_LIMIT) {
Can use a ternary here.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.h#oldcode6611
src/hydrogen-instructions.h:6611:
Whitespace change
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.h#newcode42
src/hydrogen-instructions.h:42:
unnecessary whitespace change
https://codereview.chromium.org/17568015/diff/44001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/17568015/diff/44001/src/ia32/lithium-codegen-ia32.cc#newcode4382
src/ia32/lithium-codegen-ia32.cc:4382: void
LCodeGen::ApplyCheckIf(Condition cc, LBoundsCheck* check) {
If the debug code is valuable enough to have on ia32, then it's valuable
enough to have on all platforms. Please make all platforms the same
(even though I normally am against code duplication).
https://codereview.chromium.org/17568015/diff/44001/src/v8-counters.h
File src/v8-counters.h (right):
https://codereview.chromium.org/17568015/diff/44001/src/v8-counters.h#newcode241
src/v8-counters.h:241: SC(bounds_checks_covered, V8.BoundsChecksCovered)
\
I don't know what covered and induced are supposed to mean. Do you mean
added and eliminated?
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.
