1253115 - wasm: load and store offsets

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Wasm loads and stores support constant offsets. Of particular note is that the addition of these offsets is defined to be non-wrapping, which differs from the kind of offsets we get in asm.js after folding in adds.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-unsigned-offsets.patch

This converts MAsmJSHeapAccess' offset to unsigned. Even if wasm's binary encoding is switched to use a signed encoding, it'll still be restricted to non-negative values for the foreseeable future.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-refactor.patch

This refactors several things in the asm.js heap access support.
 - Use MAsmJSHeapAccess instead of passing around various parts separately.
 - Share more code between SIMD and non-SIMD in AsmJS.cpp
 - Remove more code left behind from frontend masking analysis

Comment 3

[Dan Gohman [:sunfish]]

Attachment: wasm-offsets.patch

This patch adds support for wasm load/store offsets, though it currently falls back to MAdd for offsets that can't be folded with the current codegen support, so it doesn't have correct overflow behavior.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8726316 [details] [diff] [review]
wasm-unsigned-offsets.patch

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

::: js/src/jit/MIR.h
@@ +13823,1 @@
>          MOZ_ASSERT(o >= 0);

Can remove MOZ_ASSERT now.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8726318 [details] [diff] [review]
wasm-refactor.patch

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

nice!

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8726321 [details] [diff] [review]
wasm-offsets.patch

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

Closer!

::: js/src/asmjs/WasmIonCompile.cpp
@@ +1434,5 @@
>      }
>  
> +    uint32_t endOffset = access->endOffset();
> +    if (endOffset < offset)
> +        return false;

Ew, can you put this snippet under the "TODO Remove this" comment below?  (In general we should only be returning false in WasmIonCompile on OOM; anything we want to fail should get caught/reported during validation.)

@@ +1453,5 @@
>  
>      offset &= maskVal;
>      access->setOffset(offset);
>  
>      MDefinition* mask = f.constant(Int32Value(maskVal), MIRType_Int32);

Can you also disable masking if !f.mg().isAsmJS()?  People playing around may actually hit that.

Comment 7

[Dan Gohman [:sunfish]]

(In reply to Luke Wagner [:luke] from comment #6)
> @@ +1453,5 @@
> >  
> >      offset &= maskVal;
> >      access->setOffset(offset);
> >  
> >      MDefinition* mask = f.constant(Int32Value(maskVal), MIRType_Int32);
> 
> Can you also disable masking if !f.mg().isAsmJS()?  People playing around
> may actually hit that.

This is actually a workaround for bug 1250198; some of our backends don't implement unaligned accesses yet. I'll add a TODO comment.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2f7ef7566d
https://hg.mozilla.org/integration/mozilla-inbound/rev/590ae9fe8759
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f58f4320c5

Comment 9

[Luke Wagner [:luke]]

Attachment: fix-offset-gvn

Never needed these before b/c EAA happened after GVN, but we sure do now :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac2f7ef7566d
https://hg.mozilla.org/mozilla-central/rev/590ae9fe8759
https://hg.mozilla.org/mozilla-central/rev/c7f58f4320c5

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8727121 [details] [diff] [review]
fix-offset-gvn

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

Aha, good catch!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e696dae3026

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e696dae3026