1709209 - SIMD (all platforms), move MaskSimdShiftCount logic to the WasmIonCompile

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Yury Delendik (:yury)]

Based on https://phabricator.services.mozilla.com/D113184#inline-633780 comment:

It will be nice to introduce the following logic at WasmIonCompile instead of MaskSimdShiftCount used in packed shift functions in the MacroAssemblerX86Shared:

    MConstant* mask = MConstant::New(alloc(), Int32Value(maskBits));
    curBlock_->add(mask);
    MBitAnd* maskedShift = MBitAnd::New(alloc(), rhs, mask, MIRType::Int32);
    curBlock_->add(maskedShift);
    rhs = maskedShift;

This will remove temp usage from this functions.

Comment 1

[Lars T Hansen [:lth]]

Two points:

• the abstraction we come up with for this should be cross-platform, there should be no ifdefs in WasmIonCompile, the block above used to be guarded on !rhs->isConstant() && MustMaskShiftCountForSimd128(op), and this is still the right API
• when the masking is moved out of the macroassembler, then the macroassembler API changes, so comments must be updated AND the baseline compiler must be prepared to do the masking, using the same mechanism as Ion (ie as in the previous bullet)

Comment 2

[Yury Delendik (:yury)]

I looked at patch for ARM64 (D100659) and it is the same story there. We will need to have that from every platform.

Comment 3

[lukas.bernhard]

I've been looking into this bug and there are some points I'd like to clarify before resuming the implementation:

• Taking packedRightShiftByScalarInt64x2 (MacroAssembler-x86-shared-SIMD.cpp) as an example, is the goal to remove the call to ScratchSimd128Scope or is the goal to reduce the number of temps supplied by the caller? afaict its either one or the other, but not both.
• Regarding masking in wasmbaseline: ShiftLeftI8x16 (in WasmBaselineCompile.cpp) and alike could emit masking code by calling something like "MaskShiftOp(masm, rs, ...)". Does this sound right?
• In baseline functions such as ShiftLeftI8x16, is the generated code allowed to clobber input regs? E.g., could the masking in baseline clobber the GP reg encoding the shift scalar?
• If the masking of variable shift counts is moved to WasmIonCompile, the semantics of MWasmShiftSimd128 change. In particular, the caller of MWasmShiftSimd128 has to ensure the variable shift is masked properly, but large constants are handled in arch-specific code. Kinda feels weird, anything that should be done there?

Comment 4

[Lars T Hansen [:lth]]

Keeping the ni? for myself for now, but Yury might also be able to answer this.

Comment 5

[Yury Delendik (:yury)]

I see it like this: on each platform we have to perform the same exact operation before we do shift. And this operation (mask shift operand) complicates each shift instruction, requiring to allocate temps and scratch registers. It will be more sane to introduce e.g. MWasmMaskShiftOperand Simd128 (we need to do something about the name) that will take care of this. As noted, the semantics of MWasmShiftSimd128 will change to expect valid / in-range operand to have deterministic behavior. That said the main goal is not reduce the number of temps or remove the call to ScratchSimd128Scope.

Comment 6

[lukas.bernhard]

Maybe naive, but how about simply masking the scalar operand of MWasmShiftSimd128 with a MAnd instead of introducing MWasmMaskShiftOperandSimd128? The MAnd takes the scalar operand and a imm32, with imm32 depending on arch + shift operation.
foldsTo + congruentTo are already implemented for MAnd, so optimizations are applied as applicable.

shiftSimd128 in WasmIonCompile could look something like:

getMaskForShift(&mask, op);  // getMaskForShift returns a constant depending on arch + op
auto* masked = MAnd::New(alloc(), rhs, Imm32(mask));
auto* i = MWasmShiftSimd128::New(alloc(), lhs, masked, op);

Comment 7

[Yury Delendik (:yury)]

If this will be as effective. Currently constants loading is not as good as we want it do be. There is bug 1671873, and pcmpeqw+psrld optimization can be applicable here (so is pslld + psrld over operand?) Priorities here are performance and reduced registers pressure.

Comment 8

[Yury Delendik (:yury)]

Keep forgetting there are multiple platforms in play. Looks like the solution in comment #6 will be acceptable for ARM64 as well.

Comment 9

[lukas.bernhard]

I got it to work on my machine (passing jit-tests on x86-64). One remarks though: when shifting multiple vectors with the same variable scalar masking is done just once, however there are still redundant movd instructions. E.g. compiling the following wasm:

(module
       (func (export "f") (param v128) (param v128) (param i32) (result v128)
         (i64x2.shr_u (local.get 0) (local.get 2))
         (i64x2.shr_u (local.get 1) (local.get 2))
         (i64x2.add)
         ))

compiles to:

and $0x3F, %edi
movd %edi, %xmm15
psrlq %xmm15, %xmm0
movd %edi, %xmm15  // redundant
psrlq %xmm15, %xmm1
paddq %xmm1, %xmm0

Is removing this movd redundancy in the scope of this ticket? Otherwise, I'll push my changes. It would be nice if someone could push this to try to get results for x86-32 and arm64.

Comment 10

[Yury Delendik (:yury)]

It would be nice if someone could push this to try to get results for x86-32 and arm64.

Submit the patch -- I'll check and forward that to the try per your request.

Comment 11

[lukas.bernhard]

Attachment: Bug 1709209 - [wasm] Move SIMD masking out of codegen. r=yury

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65a50841376f
[wasm] Move SIMD masking out of codegen. r=yury

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/65a50841376f