Skip back edge creation for "do ... while(false)" loops

Skip back edge creation for "do ... while(false)" loops

Drive-by fix: Fix disassembling "prefetch" instruction on ia32

R=titzer@chromium.org

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

[Jakob Kummerow, 2014-01-09 13:20:40]

The meat of the change is to skip the VisitForControl call when the label is a
literal that evaluates to "false":

if (stmt->cond()->ToBooleanIsFalse()) {
  Goto(loop_successor);
} else {
  CHECK_BAILOUT(VisitForControl(stmt->cond(), body_exit, loop_successor));
}

The rest is just cleanup.

Please take a look.

[titzer, 2014-01-09 14:19:52]

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

https://codereview.chromium.org/131733002/diff/1/src/hydrogen.cc#newcode4331
src/hydrogen.cc:4331: ASSERT(body_exit->HasPredecessor());
I don't think this is strictly correct, e.g. if the loop condition leads to a
block being generated that ends in a deoptimize...then there may not be any
edges out of the loop.

https://codereview.chromium.org/131733002/diff/1/src/hydrogen.cc#newcode4359
src/hydrogen.cc:4359: ASSERT(body_entry->HasPredecessor());
Same here.

[Jakob Kummerow, 2014-01-09 15:01:44]

PTAL.

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

https://codereview.chromium.org/131733002/diff/1/src/hydrogen.cc#newcode4331
src/hydrogen.cc:4331: ASSERT(body_exit->HasPredecessor());
On 2014/01/09 14:19:53, titzer wrote:
> I don't think this is strictly correct, e.g. if the loop condition leads to a
> block being generated that ends in a deoptimize...then there may not be any
> edges out of the loop.

I disagree. We are very careful to ensure that all parts of the graph are
connected properly; I can't imagine any reason why this would change in the
future, making this code useful again.
But as per offline discussion, I've reverted this part of the change.

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

lgtm

On 2014/01/09 15:01:44, Jakob wrote:
> PTAL.
> 
> https://codereview.chromium.org/131733002/diff/1/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
> https://codereview.chromium.org/131733002/diff/1/src/hydrogen.cc#newcode4331
> src/hydrogen.cc:4331: ASSERT(body_exit->HasPredecessor());
> On 2014/01/09 14:19:53, titzer wrote:
> > I don't think this is strictly correct, e.g. if the loop condition leads to
a
> > block being generated that ends in a deoptimize...then there may not be any
> > edges out of the loop.
> 
> I disagree. We are very careful to ensure that all parts of the graph are
> connected properly; I can't imagine any reason why this would change in the
> future, making this code useful again.
> But as per offline discussion, I've reverted this part of the change.

(only preserving a record of my in-person statements here, not intending to
restart the discussion).

My point was that this change was actively making the code more brittle by
replacing a perfectly valid handling of a case that used to occur with an
ASSERT. So if the code changes again in the future, then we get an ASSERT
failure instead of the compiler still handling that case. There is only *one*
place in the compiler that requires that the graph be connected, and that is the
environment liveness analysis. I don't think it's a good idea to spread the
requirements of one optimization throughout the rest of the code, and I'm pretty
sure that it was exactly this tendency that led to the mess that is the starkly
change-resistant hydrogen CFG.

[Jakob Kummerow, 2014-01-09 15:20:31]

Committed patchset #2 manually as r18519 (presubmit successful).