There are only two things on the todo list (for now):
- rewrite CheckHoistability in a non recursive way
- fix the counters for eliminated checks
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.
On 2013/07/18 12:22:58, titzer wrote:
These comments are probably better located on their respective
methods.
Done.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-bounds-check-removal.cc#newcode117
src/hydrogen-bounds-check-removal.cc:117: bool* unsafe) {
On 2013/07/18 12:22:58, titzer wrote:
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.
We are going towards a solution where we iterate the dominator tree (or
anyway test dominance relations).
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) {
On 2013/07/18 12:22:58, titzer wrote:
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.
Done.
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);
On 2013/07/18 12:22:58, titzer wrote:
If this method just returns a value, then you don't need this weird
condition
and pointer.
Done.
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
On 2013/07/18 12:22:58, titzer wrote:
I don't understand why we don't match (base + constant) as well.
Because the old bounds check elimination phase already handles
"collapsing" them, and handling them here (hoisting them) would
complicate the code further.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc#newcode2002
src/hydrogen-instructions.cc:2002: if
(bitwise->right()->IsInteger32Constant()) {
On 2013/07/18 12:22:58, titzer wrote:
As per previous comments, please don't use a pointer here, when you
can change
the order of comparisons and get rid of it.
Done.
https://codereview.chromium.org/17568015/diff/44001/src/hydrogen-instructions.cc#newcode2251
src/hydrogen-instructions.cc:2251: if (result >= MAX_LIMIT) {
On 2013/07/18 12:22:58, titzer wrote:
Can use a ternary here.
Done.
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:
On 2013/07/18 12:22:58, titzer wrote:
Whitespace change
Done.
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:
On 2013/07/18 12:22:58, titzer wrote:
unnecessary whitespace change
Done.
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) {
On 2013/07/18 12:22:58, titzer wrote:
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).
Done.
https://codereview.chromium.org/17568015/diff/55001/src/hydrogen-bounds-check-removal.cc
File src/hydrogen-bounds-check-removal.cc (right):
https://codereview.chromium.org/17568015/diff/55001/src/hydrogen-bounds-check-removal.cc#newcode101
src/hydrogen-bounds-check-removal.cc:101: if (!next->is_in_loop()) {
On 2013/07/22 13:03:10, titzer wrote:
This code is now redundant with CheckLoopPaths(Block*).
Done.
https://codereview.chromium.org/17568015/diff/55001/src/hydrogen-bounds-check-removal.cc#newcode163
src/hydrogen-bounds-check-removal.cc:163: enum PathCheckResult {
On 2013/07/22 13:03:10, titzer wrote:
How about HOISTABLE, OPTIMISTICALLY_HOISTABLE, and NOT_HOISTABLE?
Done.
https://codereview.chromium.org/17568015/diff/55001/src/hydrogen-bounds-check-removal.cc#newcode180
src/hydrogen-bounds-check-removal.cc:180: PathCheckResult
CheckLoopPaths() {
On 2013/07/22 13:03:10, titzer wrote:
CheckHoistability?
Done.
https://codereview.chromium.org/17568015/diff/55001/src/hydrogen-bounds-check-removal.cc#newcode316
src/hydrogen-bounds-check-removal.cc:316: if (data->increment() <= 0)
continue;
On 2013/07/22 13:03:10, titzer wrote:
Haven't we already done a whole lot of work to deal with induction
variables
that are counting down?
It depends what we call "a whole lot of work".
What we do is handle both HAdd and HSub when we detect the induction
variable, and flip the sign of the increment in case of HSub.
And note the "for now" in the comment: I just did not want to complicate
the code further, for now.
Handling decreasing induction would need dealing with a single "base
upper bound" (the induction base), and then trying to see if the lower
bound is >= 0.
The complication is that further branches would influence the lower
bound but bit masks would still change the upper bound.
This is slightly different from what we do here (test that the single
lower bound is a non negative constant and deal with the potentially
different upper bounds, both because of masks and additional branches),
and for now I decided not to write that code.
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.
