Closed Bug 1377518 Opened 5 years ago Closed 5 years ago

Support wasm opcode prefixes

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

Prefix bytes are coming with atomics.  This bug should do only two things:

- support prefix bytes in the instruction decoder

- add an AsmJS or Mozilla prefix and move all our AsmJS opcodes into that space,
  arguably 0xFF is reserved for this though I need to track down the precise
  agreement from the last CG meeting

(Moving the atomic opcodes from the asmjs space to a new atomics space, if they are 100% compatible, will be the subject of another bug)

There are two obvious ways to do this.  The simpler one ends up just using a sparse opcode space, where eg AsmJS's "TeeGlobal" is 0xFF01 in the existing Op enum.  The main risk here is that the C++ compiler will not compile such a sparse switch well, and it does not scale well to ops that require more than two bytes.

The harder way does multi-level dispatch; dispatching first on the prefix byte and then again on the op.  This is harder because we have a fair amount of code that is not friendly to this setup.
This is the "harder" solution mentioned in comment 0.  It is a substantial change but largely mechanical.

I've kept this relatively clean by factoring out an OpBytes struct that holds the opcode bytes that have been read.  The main cost here is that the opcode reader needs to check whether to read one byte or two, though in truth our reader already did that.  The alternative, reading byte-at-a-time so that we don't have to do the check in the reader, works some of the time but becomes more complicated in other cases.
Attachment #8882704 - Flags: review?(sunfish)
Blocks: 1377576
Comment on attachment 8882704 [details] [diff] [review]
bug1377518-wasm-opcode-prefixes.patch

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

This looks like a reasonable approach to me. The committee wished to reserve 0xff for hypothetical far-future evolution purposes, so as long as we reject it on incoming wasm, it's fine to use it internally. If it ever gets assigned a standard meaning, we can pick a different unused byte.

It may even be worth asking the committee to reserve a byte specifically for use by implementations like this.

::: js/src/jit-test/tests/wasm/binary.js
@@ +444,5 @@
> +}
> +
> +// Prefixed opcodes
> +for (let prefix of [AtomicPrefix, MozPrefix]) {
> +    print(prefix);

Is this a leftover from debugging?

::: js/src/wasm/WasmBinaryIterator.h
@@ +606,5 @@
>  };
>  
>  template <typename Policy>
>  inline bool
> +OpIter<Policy>::unrecognizedOpcode(OpBytes* expr)

The OpBytes here can be const-qualified.

::: js/src/wasm/WasmValidate.h
@@ +246,5 @@
>          MOZ_ASSERT(size_t(op) < size_t(Op::Limit));
> +        return writeFixedU8(uint8_t(op));
> +    }
> +    MOZ_MUST_USE bool writeOp(MozOp op) {
> +        static_assert(size_t(MozOp::Limit) <= UINT8_MAX, "fits");

It'd be nice to have a `MOZ_ASSERT(size_t(op) < size_t(MozOp::Limit));` here, so that we have consistent paranoia with the existing `writeOp`.
Attachment #8882704 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8142a8ca666cac6d886cf00ade41df659ba617a5
Bug 1377518 - Support wasm opcode prefixes, and use the reserved FF prefix for internal asm.js opcodes. r=sunfish
https://hg.mozilla.org/mozilla-central/rev/8142a8ca666c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
This bug improved some of the compiler warnings. Thank you!

== Change summary for alert #7671 (as of July 02 2017 22:21 UTC) ==

Improvements:

  2%  compiler warnings summary linux32 debug      443.00 -> 435.00
  2%  compiler warnings summary android-4-0-armv7-api15 debug 570.00 -> 560.00
  2%  compiler warnings summary linux64 debug base-toolchains446.00 -> 439.00
  2%  compiler warnings summary linux64-stylo debug 446.00 -> 439.00
  2%  compiler warnings summary linux64 debug      446.00 -> 439.00
  1%  compiler warnings summary linux64 opt valgrind487.00 -> 481.00
  1%  compiler warnings summary linux32 opt        477.00 -> 472.00
  1%  compiler warnings summary linux64 opt base-toolchains487.00 -> 482.00
  1%  compiler warnings summary linux64-stylo opt  487.00 -> 482.00
  1%  compiler warnings summary linux64 opt        487.00 -> 482.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7671
You need to log in before you can comment on or make changes to this bug.