wasm: Implement i64 load/store

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8768856 [details] [diff] [review]
wip.patch

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

2 years ago
Created attachment 8769152 [details] [diff] [review]
folded.patch

Folded patch, to evaluate size and self-review.
Attachment #8768856 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Created attachment 8769158 [details] [diff] [review]
1-scalar-int64.patch

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

2 years ago
Created attachment 8769159 [details] [diff] [review]
2-impl.patch

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

2 years ago
Created attachment 8769160 [details] [diff] [review]
3-baseline-compiler.patch

Impl in the baseline compiler.
Attachment #8769160 - Flags: review?(lhansen)
(Assignee)

Comment 6

2 years ago
Created attachment 8769161 [details] [diff] [review]
4-tests.patch

And tests (we now pass 4 more spec tests \o/).
Attachment #8769161 - Flags: review?(sunfish)
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 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

2 years ago
Attachment #8769158 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8769160 - Flags: review?(lhansen) → review+

Comment 10

2 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
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.