Eliminatable CheckMaps replaced with if(true) or if(false).

Eliminatable CheckMaps replaced with if(true) or if(false).

R=titzer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=18592

[Igor Sheludko, 2014-01-13 11:17:03]

PTAL

[titzer, 2014-01-13 17:26:56]

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h
File src/hydrogen-flow-engine.h (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
src/hydrogen-flow-engine.h:124: if (SkipNonDominatedBlock(root, block))
continue;
You probably want a "if (block->IsUnreachable()) continue;" here.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
src/hydrogen-flow-engine.h:190: // Process all the effects of the block.
You probably want "if (member->IsUnreachable()) continue;"

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode326
src/hydrogen.cc:326: ASSERT(IsFinished());
I think you should go ahead and implement the general case (i.e. more than 2
successors).

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode345
src/hydrogen.cc:345: MarkSuccEdgeUnreachable(unreachable_succ);
You should go through all the "other" successors, and if they have only one
predecessor (namely this one), then mark them as unreachable. Normally they
wouldn't, since we are careful to have edge-split form, but check anyway.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h#newcode170
src/hydrogen.h:170: void MarkSuccEdgeUnreachable(int succ);
I think you can make this private.

[titzer, 2014-01-13 17:53:56]

On 2014/01/13 17:26:56, titzer wrote:
> https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h
> File src/hydrogen-flow-engine.h (right):
> 
>
https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
> src/hydrogen-flow-engine.h:124: if (SkipNonDominatedBlock(root, block))
> continue;
> You probably want a "if (block->IsUnreachable()) continue;" here.
> 
>
https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
> src/hydrogen-flow-engine.h:190: // Process all the effects of the block.
> You probably want "if (member->IsUnreachable()) continue;"
> 
> https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
> https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode326
> src/hydrogen.cc:326: ASSERT(IsFinished());
> I think you should go ahead and implement the general case (i.e. more than 2
> successors).
> 

As discussed in person, we don't have any control flow instructions with more
than 2 successors, so you don't have to do this.

> https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode345
> src/hydrogen.cc:345: MarkSuccEdgeUnreachable(unreachable_succ);
> You should go through all the "other" successors, and if they have only one
> predecessor (namely this one), then mark them as unreachable. Normally they
> wouldn't, since we are careful to have edge-split form, but check anyway.
> 
> https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h
> File src/hydrogen.h (right):
> 
> https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h#newcode170
> src/hydrogen.h:170: void MarkSuccEdgeUnreachable(int succ);
> I think you can make this private.

[Igor Sheludko, 2014-01-14 10:41:36]

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h
File src/hydrogen-flow-engine.h (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
src/hydrogen-flow-engine.h:124: if (SkipNonDominatedBlock(root, block))
continue;
On 2014/01/13 17:26:56, titzer wrote:
> You probably want a "if (block->IsUnreachable()) continue;" here.

Done, but a little bit differently.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen-flow-engine.h#n...
src/hydrogen-flow-engine.h:190: // Process all the effects of the block.
On 2014/01/13 17:26:56, titzer wrote:
> You probably want "if (member->IsUnreachable()) continue;"

Done.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode326
src/hydrogen.cc:326: ASSERT(IsFinished());
On 2014/01/13 17:26:56, titzer wrote:
> I think you should go ahead and implement the general case (i.e. more than 2
> successors).
It seems that there are no instructions with more than two successors.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.cc#newcode345
src/hydrogen.cc:345: MarkSuccEdgeUnreachable(unreachable_succ);
On 2014/01/13 17:26:56, titzer wrote:
> You should go through all the "other" successors, and if they have only one
> predecessor (namely this one), then mark them as unreachable. 
Normally they
> wouldn't, since we are careful to have edge-split form, but check anyway.
MarkSuccEdgeUnreachable() contains an ASSERT checking that we deal with the
edge-split form.

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/130613003/diff/1/src/hydrogen.h#newcode170
src/hydrogen.h:170: void MarkSuccEdgeUnreachable(int succ);
On 2014/01/13 17:26:56, titzer wrote:
> I think you can make this private.

Done.

[titzer, 2014-01-14 13:40:32]

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen-flow-engine.h
File src/hydrogen-flow-engine.h (right):

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen-flow-engin...
src/hydrogen-flow-engine.h:149: SetStateAt(succ, state);
This is still not quite correct, because an unreachable successor should still
have the merge of all of its incoming predecessors. I think you can just remove
this first case and it will be correct.

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen.cc#newcode329
src/hydrogen.cc:329: value,
Why not always if(true) {succ} else {1 - succ}?

[Igor Sheludko, 2014-01-14 15:28:00]

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen-flow-engine.h
File src/hydrogen-flow-engine.h (right):

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen-flow-engin...
src/hydrogen-flow-engine.h:149: SetStateAt(succ, state);
On 2014/01/14 13:40:32, titzer wrote:
> This is still not quite correct, because an unreachable successor should still
> have the merge of all of its incoming predecessors. I think you can just
remove
> this first case and it will be correct.

Done.

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/130613003/diff/130001/src/hydrogen.cc#newcode329
src/hydrogen.cc:329: value,
On 2014/01/14 13:40:32, titzer wrote:
> Why not always if(true) {succ} else {1 - succ}?

Done.

[titzer, 2014-01-14 15:32:15]

lgtm, please run the webkit tests that we have checked into V8 as well.

[Igor Sheludko, 2014-01-14 16:06:48]

Committed patchset #3 manually as r18592 (presubmit successful).

[Jakob Kummerow, 2014-01-15 10:30:19]

https://codereview.chromium.org/130613003/diff/210001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/130613003/diff/210001/src/hydrogen.cc#newcode334
src/hydrogen.cc:334: end()->DeleteAndReplaceWith(end()->ActualValue());
As discussed offline, this is a bug.

Some instructions define dependencies on control instructions (such as
HCheckMaps) because they must be scheduled after the check (i.e. must not be
hoisted past it). Replacing the control node input to such an instruction with
the check's input value is hence invalid, as doing so removes the explicit
scheduling dependency.
(At least, I think that's the reason why this CL is causing test failures.)

[titzer, 2014-01-15 10:40:04]

On 2014/01/15 10:30:19, Jakob wrote:
> https://codereview.chromium.org/130613003/diff/210001/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/130613003/diff/210001/src/hydrogen.cc#newcode334
> src/hydrogen.cc:334: end()->DeleteAndReplaceWith(end()->ActualValue());
> As discussed offline, this is a bug.
> 
> Some instructions define dependencies on control instructions (such as
> HCheckMaps) because they must be scheduled after the check (i.e. must not be
> hoisted past it). Replacing the control node input to such an instruction with
> the check's input value is hence invalid, as doing so removes the explicit
> scheduling dependency.
> (At least, I think that's the reason why this CL is causing test failures.)

Jakob is right, any uses of the control instruction need to be replaced with the
last successful check instruction (or the store that changes the map), so check
elimination must track the reason that it has determined the check to be
redundant.