Closed Bug 1253115 Opened 8 years ago Closed 8 years ago

wasm: load and store offsets

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(4 files)

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.
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.
Attachment #8726316 - Flags: review?(luke)
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
Attachment #8726318 - Flags: review?(luke)
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.
Attachment #8726321 - Flags: review?(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.
Attachment #8726316 - Flags: review?(luke) → review+
Comment on attachment 8726318 [details] [diff] [review]
wasm-refactor.patch

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

nice!
Attachment #8726318 - Flags: review?(luke) → review+
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.
Attachment #8726321 - Flags: review?(luke) → review+
(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.
Attached patch fix-offset-gvnSplinter Review
Never needed these before b/c EAA happened after GVN, but we sure do now :)
Attachment #8727121 - Flags: review?(sunfish)
Comment on attachment 8727121 [details] [diff] [review]
fix-offset-gvn

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

Aha, good catch!
Attachment #8727121 - Flags: review?(sunfish) → review+
You need to log in before you can comment on or make changes to this bug.