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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Blocks: 677337
(Assignee)

Comment 1

5 years ago
Created attachment 597006 [details] [diff] [review]
Patch

MTest was missing a type policy, so MTest(string) was lowered to LTestIAndBranch.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #597006 - Flags: review?(mrosenberg)
(Reporter)

Comment 2

5 years ago
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)
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 597823 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/98ff681bd1d7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.