Closed
Bug 1253115
Opened 8 years ago
Closed 8 years ago
wasm: load and store offsets
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(4 files)
5.63 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
72.22 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
6.15 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
(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.
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•8 years ago
|
||
Never needed these before b/c EAA happened after GVN, but we sure do now :)
Attachment #8727121 -
Flags: review?(sunfish)
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac2f7ef7566d https://hg.mozilla.org/mozilla-central/rev/590ae9fe8759 https://hg.mozilla.org/mozilla-central/rev/c7f58f4320c5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 11•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1e696dae3026
You need to log in
before you can comment on or make changes to this bug.
Description
•