1389461 - Implement new WebAssembly sign extension opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Lars T Hansen [:lth]]

These opcodes came in with the threads proposal (https://github.com/WebAssembly/threads) and are needed for full support of atomics, since the atomics don't have the variety of access flags that the regular loads and stores have.

Might land these behind a NIGHTLY flag or similar until the proposal is nailed down and we know we're ready to ship.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1389461-wasm-sign-extension-ops.patch

This patch implements the sign extension opcodes from the threads proposal, under the #ifdef introduced by bug 1389471 (which has not landed yet and sits underneath the present patch in local queue).

Here we have changes to:

- wasm text->binary and binary->text
- wasm verifier
- wasm ion and baseline compilers
- mir/lir/assembler
- wasm test infrastructure and test cases

I renamed the existing MSignExtend as MSignExtendInt32 (because that's what it is) and also LSignExtend -> LSignExtendInt32 and RSignExtend -> RSignExtendInt32.  Since MSignExtendInt32 had no constant folding machinery I added some.  I then added MSignExtendInt64/LSignExtendInt64, though without recover machinery.

Most of this feels pretty straightforward.  I'm a little shaky on the LIR/codegen stuff for 64-bit so extra attention there is probably welcome.

Comment 2

[Lars T Hansen [:lth]]

"try" seems increasingly useless whatwith all the unfixed Windows failures, but ignoring that bloodbath this patch compiles, passes the new tests, and does not regress anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37bba91b79f8475855c145003cc1e01da51a7db5

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8897447 [details] [diff] [review]
bug1389461-wasm-sign-extension-ops.patch

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

Thanks, looks good to me! There's an opportunity to merge the new MIR node (for i64) with MExtendInt32ToInt64. Codegen looks good; for x64/x86 I've compared with clang/g++ and they match. For ARM, it looks reasonable too (there's no obvious instruction to sign-extend to int64 in one shot).

::: js/src/jit/MIR.h
@@ +6254,5 @@
> +        if (!ins->isSignExtendInt32())
> +            return false;
> +        if (ins->toSignExtendInt32()->mode_ != mode_)
> +            return false;
> +        return congruentIfOperandsEqual(ins);

(I like to do it the other way, testing congruentIfOperandsEqual first, because then it removes the op() check; there could be an argument about eliminating the most common cases first by comparing the modes. No strong opinion here, but a light suggestion to enhance readability, if you want)

@@ +6268,5 @@
>  
> +    ALLOW_CLONE(MSignExtendInt32)
> +};
> +
> +class MSignExtendInt64

Semantically, it'd be correct to merge this with MExtendInt32ToInt64, an i32->i64 extension is all about the sign. That being said, it will introduce more configurations of this MIR node (i32->i64 can be unsigned, plus the lowering/codegen will branch over the input's MIR type). But it would tidy it up. Especially if unsigned sign extensions are happening later. It would also be a nice economy of concepts, and less confusing to remember which one is what.

::: js/src/jit/arm/Lowering-arm.cpp
@@ +1057,5 @@
>  
>      lir->setDef(0, def);
>  }
> +
> +

nit: two blank lines, please keep only one

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +756,5 @@
> +LIRGeneratorMIPSShared::visitSignExtendInt64(MSignExtendInt64* ins)
> +{
> +    MOZ_CRASH("NYI : SignExtendInt64");
> +}
> +

nit: trailing blank line

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +1064,5 @@
> +    MOZ_ASSERT(input.high == edx);
> +    MOZ_ASSERT(output.high == edx);
> +    switch (lir->mode()) {
> +      case MSignExtendInt64::Byte:
> +        masm.move8SignExtend(input.low, output.low);

Maybe we could call all of them eax, to be more transparent?

@@ +1072,5 @@
> +        break;
> +      case MSignExtendInt64::Word:
> +        break;
> +    }
> +    masm.cdq();

Fwiw, gcc -O3 does the same thing. Clang does the first sign extension, then moves output.low to output.high and sign shift output.high by 0x1f.

::: js/src/jit/x86/Lowering-x86.cpp
@@ +724,5 @@
> +
> +void
> +LIRGeneratorX86::visitSignExtendInt64(MSignExtendInt64* ins)
> +{
> +    // Here we'll end up using cdq which requires input and output in (edx,eax).

This matches what the lowering of ExtendInt32ToInt64 does, so this looks good to me.

::: js/src/wasm/WasmBinaryConstants.h
@@ +306,4 @@
>      F32ReinterpretI32                    = 0xbe,
>      F64ReinterpretI64                    = 0xbf,
>  
> +    // Sign extension

Don't they need to be protected by ENABLE_WASM_THREADS too, otherwise switch statements may complain under !ENABLE_WASM_THREADS that cases aren't handled?

::: js/src/wasm/WasmBinaryToAST.cpp
@@ +1257,5 @@
> +      case uint16_t(Op::I32Extend8S):
> +        if (!AstDecodeConversion(c, ValType::I32, ValType::I32, Op(op.b0)))
> +            return false;
> +        break;
> +      case uint16_t(Op::I32Extend16S):

nit: this case can be smashed with the one above

@@ +1269,5 @@
> +      case uint16_t(Op::I64Extend16S):
> +        if (!AstDecodeConversion(c, ValType::I64, ValType::I64, Op(op.b0)))
> +            return false;
> +        break;
> +      case uint16_t(Op::I64Extend32S):

nit: these 3 cases can be grouped

::: js/src/wasm/WasmIonCompile.cpp
@@ +682,5 @@
> +              case 1:
> +                ins = MSignExtendInt32::New(alloc(), op, MSignExtendInt32::Byte);
> +                break;
> +              case 2:
> +                ins = MSignExtendInt32::New(alloc(), op, MSignExtendInt32::Half);

light suggestion, here and below: the inner switch could just define the MSignExtendInt32::mode and then the ins could be constructed just thereafter, for a shorter switch case

@@ +2300,4 @@
>  }
>  
>  static bool
> +EmitSignExtend(FunctionCompiler& f, uint32_t srcSize, uint32_t targetSize)

An alternative API would be to pass ValType instead of targetSize, move the MSignExtendInt32/Int64::Mode outside these MIR classes and use a single enum class for mode instead of two, and pass an instance of Mode instead of srcSize here. The signExtend method above would also become trivial then. What do you think?

@@ +2304,5 @@
> +{
> +    MDefinition* input;
> +    ValType type = targetSize == 4 ? ValType::I32 : ValType::I64;
> +    if (!f.iter().readConversion(type, type, &input))
> +            return false;

nit: the level of indent here is too damn high

Comment 4

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8897447 [details] [diff] [review]
> bug1389461-wasm-sign-extension-ops.patch
> 
> Review of attachment 8897447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks good to me! There's an opportunity to merge the new MIR node
> (for i64) with MExtendInt32ToInt64.

I tried this earlier and it was not a good fit - instead of obviously-correct code (even if with some duplication) it became messy to handle the various combinations of legal input and output sizes and signedness, without implementing all combinations (which I had no interest in).  For the same reason I decided to keep different enums in SignExtendInt32 and SignExtendInt64, since only the latter has 'Word'.

Took your other suggestions, though.  Thanks!  Will carefully test compilation with the feature disabled too...

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b755618d7c13
WebAssembly sign extension opcodes.  r=bbouvier

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b755618d7c13