[Interpreter] Removes unnecessary jumps and dead code from If and loops.

[Interpreter] Removes unnecessary jumps and dead code from If and loops.

Adds an optimization to not emit unnecessary jumps and dead code in If,
For, While, and do-while statments. When the value of condition is known
at compile time, the code is emitted only for the paths that can be taken.
For example, when the condition is known to be true in an if statmenet
only then block is generated.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/77c19034f482c40d215a437751a08854b36c5579
Cr-Commit-Position: refs/heads/master@{#31715}

[mythria, 2015-10-29 17:23:22]

mythria@chromium.org changed reviewers:
+ oth@chromium.org, rmcilroy@chromium.org

[mythria, 2015-10-29 17:23:23]

Could you please review my changes.

Thanks,
Mythri

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:495: 
Are there any other cases other than the condition being true/false? I will
update the comment based on your feedback.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:593: return;
Here, I am not sure if we have to call builder().EnterBlock() and
builder.LeaveBlock(). Since, we are not generating any code, I don't think it is
necessary. We maintain the same state as if we never saw a while statement.

[oth, 2015-10-30 09:46:19]

lgtm. A few minor comments to address, but looks good and output is definitely
improved.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:495: 
On 2015/10/29 17:23:23, mythria wrote:
> Are there any other cases other than the condition being true/false? I will
> update the comment based on your feedback.

I believe these are the only instances of interest.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:593: return;
On 2015/10/29 17:23:23, mythria wrote:
> Here, I am not sure if we have to call builder().EnterBlock() and
> builder.LeaveBlock(). Since, we are not generating any code, I don't think it
is
> necessary. We maintain the same state as if we never saw a while statement. 

Definitely not necessary here.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:624: // If the condition is known to be
false, no need of generating
s/no need of generating/there is no need to generate the/

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:630: if (stmt->cond() &&
!stmt->cond()->ToBooleanIsTrue()) {
For readability I'd suggest using stmt->cond()->ToBooleanIsFalse() rather than
!stmt->cond()->ToBooleanIsTrue().

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:639: if (stmt->cond() &&
!stmt->cond()->ToBooleanIsTrue()) {
Ditto.

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:1260: B(LdaUndefined),     //
It looks like our logic for detecting when to emit return hasn't worked here.
Can you a todo here to avoid emitting the second return.

[mythria, 2015-10-30 15:07:47]

I have addressed review comments except for one in bytecode-generator.cc. I
could not remove !stmt->cond()->ToBooleanIsTrue in the condition.  Please review
my changes and let me know if you have any suggestions.

Thanks and Regards,
Mythri

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:624: // If the condition is known to be
false, no need of generating
On 2015/10/30 09:46:19, oth wrote:
> s/no need of generating/there is no need to generate the/

Done.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:630: if (stmt->cond() &&
!stmt->cond()->ToBooleanIsTrue()) {
On 2015/10/30 09:46:19, oth wrote:
> For readability I'd suggest using stmt->cond()->ToBooleanIsFalse() rather than
> !stmt->cond()->ToBooleanIsTrue().

!stmt->cond()->ToBooleanIsTrue could also mean that the condition is not known
at compile time. I could not think of anyway to avoid
!stmt->cond()->ToBooleanIsTrue() here. Please let me know if you have any
suggestions.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:639: if (stmt->cond() &&
!stmt->cond()->ToBooleanIsTrue()) {
On 2015/10/30 09:46:18, oth wrote:
> Ditto.

Here, I changed the condition to avoid using !stmt->cond()->ToBooleanIsTrue().

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:1260: B(LdaUndefined),     //
On 2015/10/30 09:46:19, oth wrote:
> It looks like our logic for detecting when to emit return hasn't worked here.
> Can you a todo here to avoid emitting the second return.

I am not sure if it is easy to eliminate return here. We have to do inter block
optimization here. The earlier return was in a different block.

[oth, 2015-10-30 15:31:34]

Thanks for the changes and clarifications. lgtm.

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:630: if (stmt->cond() &&
!stmt->cond()->ToBooleanIsTrue()) {
On 2015/10/30 15:07:47, mythria wrote:
> On 2015/10/30 09:46:19, oth wrote:
> > For readability I'd suggest using stmt->cond()->ToBooleanIsFalse() rather
than
> > !stmt->cond()->ToBooleanIsTrue().
> 
> !stmt->cond()->ToBooleanIsTrue could also mean that the condition is not known
> at compile time. I could not think of anyway to avoid
> !stmt->cond()->ToBooleanIsTrue() here. Please let me know if you have any
> suggestions.

Sorry, yes, reader error.

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:1260: B(LdaUndefined),     //
On 2015/10/30 15:07:47, mythria wrote:
> On 2015/10/30 09:46:19, oth wrote:
> > It looks like our logic for detecting when to emit return hasn't worked
here.
> > Can you a todo here to avoid emitting the second return.
> 
> I am not sure if it is easy to eliminate return here. We have to do inter
block
> optimization here. The earlier return was in a different block. 

Yes.

[rmcilroy, 2015-10-30 15:49:53]

A couple of nits, but lgtm, thanks!

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:566: Visit(stmt->body());
nit - add line of whitespace here between body and condition check

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:595: Visit(stmt->body());
nit - add a line of whitespace here  between body and condition check

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:634: if ((stmt->cond() == nullptr) ||
stmt->cond()->ToBooleanIsTrue()) {
nit - you removed the '== nullptr' above and added it here. Just one or the
other (I'd just remove the '== nullptr')

[mythria, 2015-11-02 10:46:07]

Thanks, I addressed all the comments. I will rebase it and commit.

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:566: Visit(stmt->body());
On 2015/10/30 15:49:53, rmcilroy wrote:
> nit - add line of whitespace here between body and condition check

Done.

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:595: Visit(stmt->body());
On 2015/10/30 15:49:53, rmcilroy wrote:
> nit - add a line of whitespace here  between body and condition check

Done.

https://codereview.chromium.org/1414193006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:634: if ((stmt->cond() == nullptr) ||
stmt->cond()->ToBooleanIsTrue()) {
On 2015/10/30 15:49:53, rmcilroy wrote:
> nit - you removed the '== nullptr' above and added it here. Just one or the
> other (I'd just remove the '== nullptr')

Done.

[mythria, 2015-11-02 12:12:22]

I added a test for generating JumpIfToBoolean for If statements. The earlier
tests were optimized out because of this cl.

Thanks and Regards,
Mythri

[mythria, 2015-11-02 15:22:54]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-11-02 15:22:54]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1414193006/#ps80001
(title: "Adds a test for generating JumpIfToBoolean for If statement. The
earlier tests were optimized out b…")

[commit-bot: I haz the power, 2015-11-02 15:23:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414193006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414193006/80001

[commit-bot: I haz the power, 2015-11-02 15:24:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-02 15:24:55]

Patchset 5 (id:??) landed as
https://crrev.com/77c19034f482c40d215a437751a08854b36c5579
Cr-Commit-Position: refs/heads/master@{#31715}