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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier]]

Attachment: fix-bug-941877-tests.patch

It looks rather correct with this patch but I want to make sure that I didn't change the tests semantics.

Comment 1

[Dan Gohman [: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.

Comment 2

[Benjamin Bouvier [:bbouvier]]

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).

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e808372ebbd4