Closed Bug 1016951 Opened 6 years ago Closed 6 years ago

OdinMonkey: jit-tests/tests/asm.js/bug941877.js contains invalid asm.js code

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file)

It looks rather correct with this patch but I want to make sure that I didn't change the tests semantics.
Attachment #8430020 - Flags: review?(sunfish)
Comment on attachment 8430020 [details] [diff] [review]
fix-bug-941877-tests.patch

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

::: js/src/jit-test/tests/asm.js/bug941877.js
@@ +75,5 @@
>      {
>          x = x|0;
>          y = y|0;
>          var t = 0;
> +        t = (((x >> 0) % (y >> 0))|0) > -2;

Is there a reason for using >>0 instead of |0 ?

@@ +97,5 @@
>          return t|0;
>      }
>      return f;
>  }
> +var compiled = asmCompile(USE_ASM + FunctionBody(test4));

What's the reason for using USE_ASM + ... instead of leaving the "use asm" in test4 itself?

@@ -93,5 @@
>      {
>          x = x|0;
>          y = y|0;
> -        var t = 0;
> -        t = ((x>>>0) % (y>>>0)) * 1.01;

This used to work? Crazy.
Attachment #8430020 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e808372ebbd4


(In reply to Dan Gohman [:sunfish] from comment #1)
> Comment on attachment 8430020 [details] [diff] [review]
> fix-bug-941877-tests.patch
> 
> Review of attachment 8430020 [details] [diff] [review]:
> -----------------------------------------------------------------
> Is there a reason for using >>0 instead of |0 ?
No indeed, changed it to |0 as it's simpler.

> 
> @@ +97,5 @@
> >          return t|0;
> >      }
> >      return f;
> >  }
> > +var compiled = asmCompile(USE_ASM + FunctionBody(test4));
> 
> What's the reason for using USE_ASM + ... instead of leaving the "use asm"
> in test4 itself?

If we leave the "use asm" directive directly in the function body, the interpreter will try to compile it as an asm.js module directly, falling back to the interpreter if the module isn't valid, without any warning. asmCompile makes sure that the warning "Successfully compiled asm.js module" is caught. We could leave the "use asm" directive in the function body *and* use asmCompile, but it means that compilation would be done twice (once as soon as the function is parsed, a second time in the eval present in asmCompile).
https://hg.mozilla.org/mozilla-central/rev/e808372ebbd4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.