WASM.SIMD: Trunc & Conv ops for 64x2 types #4074
Conversation
arunetm-zz
force-pushed
the
arunetm/wasm.simd.trunc64x2
branch
from
257d0bf
to
5d48354
Compare
|
@Cellule, @MikeHolman please review. |
| @@ -4672,7 +4672,6 @@ IRBuilderAsmJs::BuildInt64x2_3(Js::OpCodeAsmJs newOpcode, uint32 offset, BUILD_S | |||
| void | |||
| IRBuilderAsmJs::BuildInt64x2_2(Js::OpCodeAsmJs newOpcode, uint32 offset, BUILD_SIMD_ARGS_REG2) | |||
| { | |||
| Assert(newOpcode == Js::OpCodeAsmJs::Simd128_Neg_I2); | |||
There was a problem hiding this comment.
I would prefer that you add the new possible opcode in the assert.
It helps to figure out which opcode produces which IR
lib/Backend/LowerMDSharedSimd128.cpp
Outdated
| @@ -772,6 +772,18 @@ IR::Instr* LowererMD::Simd128LowerUnMappedInstruction(IR::Instr *instr) | |||
| case Js::OpCode::Simd128_FromFloat32x4_U4: | |||
| return Simd128LowerUint32x4FromFloat32x4(instr); | |||
|
|
|||
| case Js::OpCode::Simd128_FromInt64x2_D2: | |||
| EmitSimdConversion(instr, IR::HelperSimd128ConvertSD2); | |||
There was a problem hiding this comment.
Since you are going to remove the Instr. You could pass the dst and src1 to EmitSimdConversion like
EmitSimdConversion(instr->UnlinkDst(), instr->UnlinkSrc1(), IR::Helper...);;
This way, we won't have to clone 2 operands that are about to be freed.
There was a problem hiding this comment.
Ah thanks. Will do,
| @@ -3803,13 +3803,15 @@ namespace Js | |||
| case AsmJsRetType::Uint32x4: | |||
| case AsmJsRetType::Uint16x8: | |||
| case AsmJsRetType::Uint8x16: | |||
| #if _WIN32 || _WIN64 //WASM.SIMD ToDo: Enable thunk for Xplat | |||
There was a problem hiding this comment.
_WIN32 is good enough to detect that we're building for Windows
There was a problem hiding this comment.
I wonder though if we should just disable ENABLE_WASM_SIMD on xplat ?
There was a problem hiding this comment.
We can, but his way will allow us to ensure that the rest of the simd code is xplat compliant until we fix and test the thunks. Would it make sense to keep this as is?
Will do the _WIN32 check alone. I wasn't sure if that was enough until now. Thanks!
There was a problem hiding this comment.
Oh you're right, it makes sense to at least try to compile on xplat. ignore ENABLE_WASM_SIMD then
|
@Cellule could you help to sign off this change if it looks OK? |
lib/Backend/LowerMDSharedSimd128.cpp
Outdated
| @@ -915,14 +915,13 @@ IR::Instr* LowererMD::Simd128CanonicalizeToBools(IR::Instr* instr, const Js::OpC | |||
|
|
|||
| void LowererMD::EmitSimdConversion(IR::Instr *instr, IR::JnHelperMethod helper) | |||
| { | |||
| IR::Opnd* dst = instr->GetDst(); | |||
There was a problem hiding this comment.
I think I would prefer if the function doing the Unlink is the same freeing the instr.
It makes it more clear that the opnd cannot be used in between without looking at the implementation.
I recommend changing the signature of this function to EmitSimdConversion(IR::Opnd* dst, IR::Opnd* src, IR::JnHelperMethod helper) and call unlink at the call site
There was a problem hiding this comment.
we still need to pass the instr as an argument to EmitSimdConversion because of its use in loading helper memref arguments.
Shall we move removeInstr() to be inside EmitSimdConversion?
There was a problem hiding this comment.
You can move the remove inside this function if you prefer.
The instr is only used as a reference to where to insert the argument.
Usually (well sometimes) we call it insertBeforeInstr to mean that we don't actually operate on that instr, but we need to insert new ones before it.
There was a problem hiding this comment.
That makes sense. Thanks @Cellule . Please take a look.
|
@Cellule can you take a look. |
arunetm-zz
force-pushed
the
arunetm/wasm.simd.trunc64x2
branch
from
4f54ba5
to
e12dac3
Compare
|
squashed commits for merge. |
Truncation & conversion operstions for 64x2 types.
Xplat link error fixes