Closed
Bug 1283177
Opened 8 years ago
Closed 8 years ago
wasm: Implement i64 load/store
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(5 files, 1 obsolete file)
92.37 KB,
patch
|
Details | Diff | Splinter Review | |
15.89 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
50.47 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
8.09 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
14.53 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
This is a WIP patch for x64. It works nicely by reusing LWasmLoad/LWasmStore on x64 (because a GPR also is an Int64Register), but to make it nicer for other platforms, we need to create a new LIR node that will define/take an Int64Register and use a separate codegen function.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Folded patch, to evaluate size and self-review.
Attachment #8768856 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
This adds Scalar::Int64 and plugs it in in several switch on types (including NYI crashes on x86).
Attachment #8769158 -
Flags: review?(luke)
Assignee | ||
Comment 4•8 years ago
|
||
The actual implementation. Sorry this is a bit lengthy, but this also implements the correct disassembly, and changes a bit the API of the Disassembler, so it was hard to split into smaller patches after the fact.
Attachment #8769159 -
Flags: review?(sunfish)
Assignee | ||
Comment 5•8 years ago
|
||
Impl in the baseline compiler.
Attachment #8769160 -
Flags: review?(lhansen)
Assignee | ||
Comment 6•8 years ago
|
||
And tests (we now pass 4 more spec tests \o/).
Attachment #8769161 -
Flags: review?(sunfish)
Comment 7•8 years ago
|
||
Comment on attachment 8769159 [details] [diff] [review] 2-impl.patch Review of attachment 8769159 [details] [diff] [review]: ----------------------------------------------------------------- \o/ i64 load/store opens up a bunch more code we can run! ::: js/src/jit/shared/CodeGenerator-shared-inl.h @@ +335,5 @@ > if (!alloc.isConstant()) { > op = OtherOperand(ToRegister(alloc).encoding()); > } else { > + // x86 doesn't allow encoding an imm64 to memory move; the value > + // is truncated anyways. Nit: for consistency with wasm terms, s/truncated/wrapped/. ::: js/src/jit/shared/LIR-shared.h @@ +7678,5 @@ > } > }; > > +template<size_t Defs, size_t Temp> > +class LWasmLoadBase : public LInstructionHelper<Defs, 1, Temp> Can you add a brief comment here just saying "this is a base class for the wasm load classes" or so? My first thought was that "base" was referring to a component of the load address. ::: js/src/jit/x64/BaseAssembler-x64.h @@ +672,5 @@ > + void movzbq_mr(int32_t offset, RegisterID base, RegisterID index, int scale, RegisterID dst) > + { > + spew("movzbq " MEM_obs ", %s", ADDR_obs(offset, base, index, scale), GPReg64Name(dst)); > + m_formatter.twoByteOp64(OP2_MOVZX_GvEb, offset, base, index, scale, dst); > + } Minor x86 optimization: movzbq can always be replaced by movzbl to a 32-bit destination, because x64 zero-extends it to 64 bits, and movzbl is one byte smaller :-). Just as cmp_ri optimizes to test_rr elsewhere in this file, it'd be nice to have the movzbq functions here call the corresponding movzbl functions instead of encoding an actual movzbq. @@ +694,5 @@ > + void movzwq_mr(int32_t offset, RegisterID base, RegisterID index, int scale, RegisterID dst) > + { > + spew("movzwq " MEM_obs ", %s", ADDR_obs(offset, base, index, scale), GPReg64Name(dst)); > + m_formatter.twoByteOp64(OP2_MOVZX_GvEw, offset, base, index, scale, dst); > + } And the same applies to movzwq, with movzwl being smaller. ::: js/src/jit/x64/Lowering-x64.cpp @@ +172,5 @@ > case Scalar::Uint32: > + valueAlloc = useRegisterOrConstantAtStart(value); > + break; > + case Scalar::Int64: > + // No way to encode in x64 a int64-to-memory move. Comment wording nit ;): "No way to encode an int64-to-memory move on x64".
Attachment #8769159 -
Flags: review?(sunfish) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8769161 [details] [diff] [review] 4-tests.patch Review of attachment 8769161 [details] [diff] [review]: ----------------------------------------------------------------- Woo! More spec tests passing :-). ::: js/src/jit-test/tests/wasm/spec/memory.wast @@ +32,5 @@ > "data segment not disjoint and ordered" > ) > > ;; Test alignment annotation rules > +;; TODO Tests being debated on the spec repo. If you want to link to the discussion, it's https://github.com/WebAssembly/spec/issues/217
Attachment #8769161 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8769158 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8769160 -
Flags: review?(lhansen) → review+
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea9c31e5f08 Add Scalar::Int64; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/39bfb8b9c58f wasm: Implement int64 load/stores on x64; r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/693a7e678b23 Add int64 load/store support to BaselineCompiler; r=lth https://hg.mozilla.org/integration/mozilla-inbound/rev/54e0618d9994 Tests; r=sunfish
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ea9c31e5f08 https://hg.mozilla.org/mozilla-central/rev/39bfb8b9c58f https://hg.mozilla.org/mozilla-central/rev/693a7e678b23 https://hg.mozilla.org/mozilla-central/rev/54e0618d9994
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•