add support for genReg1/genReg2->SIMD8 store on x86 windows. #52581
Conversation
sandreenko
added
the
area-CodeGen-coreclr
src/coreclr/jit/codegenxarch.cpp
Outdated
There was a problem hiding this comment.
We don't use the last argument on x86 except for check that this is not a byte register. On other platforms, we need type only for size reasons (emitActualTypeSize(type)) so it does not really matter if we do inst_RV_RV(ins_Copy(targetReg, TYP_FLOAT), targetReg, reg0, TYP_INT) or inst_RV_RV(ins_Copy(targetReg, TYP_FLOAT), targetReg, reg0, TYP_FLOAT) here. However, looking at the other examples I think it expects the type of the source operand, not the dest.
Example:
runtime/src/coreclr/jit/simdcodegenxarch.cpp
Line 535 in 9f7abf5
but it would be nice to get a confirmation.
There was a problem hiding this comment.
Its meant to be used as inst_RV_RV(ins_Copy(srcReg, tgtType), tgtReg, srcReg, srcType)
For ins_Copy(srcReg, tgtType), its validated that srcReg and tgtType are "different" (meaning one is floating-point and one is integral). The tgtType is then used to determine if int->float or float->int and the correct instruction is selected (this is just movd on x86, but fmov or mov on arm64).
For srcType, this is merely used to pick the right emit attribute so that encoding/disassembly will be correct. In the case of int32<->float it doesn't matter since both are EA_4BYTE, but the typical usage so far has just been srcType.
There was a problem hiding this comment.
@sandreenko - Could you please update the method definition of inst_RV_RV() and rename the parameter from type -> srcType in that case, for future maintainability?
There was a problem hiding this comment.
@sandreenko - Could you please update the method definition of inst_RV_RV() and rename the parameter from type -> srcType in that case, for future maintainability?
Sure
There was a problem hiding this comment.
Sorry, I think I didn't explain enough. To clarify its not necessarily srcType for all instructions, just for the ins_Copy(srcReg, tgtType) instructions.
There can be instructions where the emitAttr needs to be from the dstType or where its something else. It really depends on the particular instruction and what attribute it needs to correctly encode itself.
|
PTAL @echesakov , @dotnet/jit-contrib . The change is similar to #46899 |
|
/azp run runtime-coreclr crossgen2-composite the pipeline is very red, but lets see if |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I was able to repro it without crossgen2 using the same test with Ready for review. Note that crossgen2 throws away the produces code and when we corerun the test we compile |
|
Thanks Sergey for sharing the additional details. Have you been by any chance able to identify what is the reason the Crossgen2 code for the method is not being used at runtime? Do we skip it in Crossgen2 compilation? (Probably not, otherwise we wouldn't be hitting the SIMD JIT assertion.) Is that getting thrown out at runtime by some of the method fixup checks? |
I see the method in the "composite-r2r.dll" r2rdump, thanks for providing the command for it. So runtime throws it away, not sure why. A quick debugging shows that it is rejected here: runtime/src/coreclr/vm/prestub.cpp Lines 393 to 394 in 31c5a7c this condition returns false so we don't try to load the R2R code. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/jit/instr.cpp
Outdated
| // Arguments: | ||
| // ins - the instruction to generate; | ||
| // reg1 - the first register to use, the dst for most instructions; | ||
| // tree - the second register to use, the src for most instructions; |
|
The tests passed in https://dev.azure.com/dnceng/public/_build/results?buildId=1136599&view=results, "6 failing and 96 successful checks" is just another infra bug. |
ghost
locked as resolved and limited conversation to collaborators
6 participants
Fixes #51506