Last Comment Bug 726180 - IonMonkey: bad code generated for if (a || b)
: IonMonkey: bad code generated for if (a || b)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: 677337
  Show dependency treegraph
 
Reported: 2012-02-10 14:48 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-02-17 01:49 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reduced test case that fails elsewhere. (231 bytes, application/javascript)
2012-02-10 14:48 PST, Marty Rosenberg [:mjrosenb]
no flags Details
Patch (6.24 KB, patch)
2012-02-14 06:51 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (7.06 KB, patch)
2012-02-16 08:11 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-02-10 14:48:21 PST
Created attachment 596176 [details]
reduced test case that fails elsewhere.

For whatever reason, this initially only appeared on my machine in truthies.js, but I've reduced it, and it now reproduces on other people's repositories.
Comment 1 Jan de Mooij [:jandem] 2012-02-14 06:51:32 PST
Created attachment 597006 [details] [diff] [review]
Patch

MTest was missing a type policy, so MTest(string) was lowered to LTestIAndBranch.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-02-14 19:29:55 PST
Comment on attachment 597006 [details] [diff] [review]
Patch

Review of attachment 597006 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks fine (I probably shouldn't review too much of the lowering phase, since I've never seen the code before, but I am concerned about the code that is generated by the builder.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-02-14 19:37:01 PST
I suspect that the code that is generated by the builder is wrong, but I don't know enough about JS to get it to show anything obvious.
The code that is currently generated for 
if (a || b) {
    return 1;
}
return 0;
is (In pseudo assembly)

block 1:
a_ <- toBool(a);
if (a_) goto: 3
goto: 2

block 2:
tmp <- b
goto: 4

block 3:
tmp <- a
goto: 4

block 4:
tmp_ <- toBool(tmp)
if (tmp_) goto ret_1
goto ret_0

The fact that when the test should short circuit we are testing the first element twice seems quite wrong to me, but it currently seems to be nothing more than an efficency issue.
Comment 4 Jan de Mooij [:jandem] 2012-02-14 23:42:33 PST
(In reply to Marty Rosenberg [:Marty] from comment #3)
> 
> The fact that when the test should short circuit we are testing the first
> element twice seems quite wrong to me, but it currently seems to be nothing
> more than an efficency issue.

It's based on the bytecode we emit (the interpreter and JM do the same). It's okay to test the LHS twice since ValueToBoolean has no observable side-effects.

IonBuilder could optimize it by keeping track of branches or something but I think we should wait with that until we have better fuzz/test coverage.
Comment 5 Jan de Mooij [:jandem] 2012-02-16 08:11:13 PST
Created attachment 597823 [details] [diff] [review]
Patch

Asking for another review per comment 2.
Comment 6 David Anderson [:dvander] 2012-02-16 15:32:59 PST
Comment on attachment 597823 [details] [diff] [review]
Patch

Review of attachment 597823 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/TypePolicy.cpp
@@ +172,5 @@
> +      case MIRType_String:
> +      {
> +        MStringLength *length = MStringLength::New(op);
> +        ins->block()->insertBefore(ins, length);
> +        ins->replaceOperand(0, length);

At first I wasn't sure whether this was the right place, but I think it is, since the goal is to convert from string to boolean.
Comment 7 Jan de Mooij [:jandem] 2012-02-17 01:49:40 PST
http://hg.mozilla.org/projects/ionmonkey/rev/98ff681bd1d7

Note You need to log in before you can comment on or make changes to this bug.