Closed Bug 726180 Opened 9 years ago Closed 9 years ago

IonMonkey: bad code generated for if (a || b)

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
MTest was missing a type policy, so MTest(string) was lowered to LTestIAndBranch.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #597006 - Flags: review?(mrosenberg)
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.
Attachment #597006 - Flags: review?(mrosenberg)
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.
(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.
Attached patch PatchSplinter Review
Asking for another review per comment 2.
Attachment #597006 - Attachment is obsolete: true
Attachment #597823 - Flags: review?(dvander)
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.
Attachment #597823 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/98ff681bd1d7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.