[RyuJIT] Correct names of AVX that stand for AVX2 #13879
Conversation
fiigii
changed the title
There was a problem hiding this comment.
LGTM. I don't think the JIT ever generates AVX (but not AVX2) instructions currently, and certainly not for SIMD.
@CarolEidt comments?
@BruceForstall I think this codegen strategy should be changed when we enable Intel hardware intrinsics, which allows developers to use AVX intrinsics on non-AVX2 machine (Sandy Bridge and Ivy Bridge). Under the current codegen strategy, mixing scalar floating-point calculations with AVX intrinsics may trigger AVX-SSE transition penalties. |
|
As I mentioned in the intrinsics discussion, we will have to change our strategy. We chose to use AVX2 rather than AVX because the AVX2 support for 256-bit vectors was more complete. However, if we are going to broadly support AVX intrinsics, then I believe that we need to:
I'm not comfortable with this change as it is, because 1) I don't think it improves anything, and 2) it just leads to further confusion. What we need is to clarify (and separately address) the question of the encoding and available instruction set (i.e. the actual target hardware) vs. the size of |
@CarolEidt Agree with this new codegen strategy. We should generate VEX-encoding instructions that operate over xmm registers once AVX is supported by underlying hardware. But only enable 256-bit
We have to change something because the current RyuJIT uses "AVX" and "AVX2" interchangeably and #14020 requires |
| @@ -1158,7 +1158,7 @@ void CodeGen::genSIMDIntrinsic32BitConvert(GenTreeSIMD* simdNode) | |||
| getEmitter()->emitIns_R_R_I(INS_pinsrw, emitTypeSize(TYP_INT), tmpReg, tmpIntReg, 3); | |||
| } | |||
| #endif | |||
| if (compiler->getSIMDInstructionSet() == InstructionSet_AVX) | |||
| if (compiler->getSIMDInstructionSet() == InstructionSet_AVX2) | |||
| { | |||
| inst_RV_RV(INS_vpbroadcastd, tmpReg, tmpReg, targetType, emitActualTypeSize(targetType)); | |||
| } | |||
There was a problem hiding this comment.
@CarolEidt For example, once we distinguish InstructionSet_AVX and InstructionSet_AVX2, we will have to change here because vpbroadcastd is an AVX2 instruction.
I agree that changes are needed, but I don't think this is an appropriate change. Instead, we should:
There may be other changes required as well. |
|
@CarolEidt Got it, I will close this PR and provide a better solution. |
|
Move to #14065 |
Now, the name of
AVXactually stands for AVX2 instruction set somewhere in RyuJIT. Because we introducedSystem.Runtime.Intrinsics.X86in #13576 , RyuJIT should distinguish AVX and AVX2 ISA.Currently, RyuJIT SIMD and floating point codegen emit AVX instructions (VEX encoding) when AVX2 is available, so RyuJIT uses "AVX" and "AVX2" interchangeably which can cause confusion with
Avxintrinsics be introduced. But intrinsics in classSystem.Runtime.Intrinsics.X86.Avxmay change this strategy, so we should distinguish AVX and AVX2 in RyuJIT (names in vm side are correct).During implementing
System.Runtime.Intrinsics.X86, I foundInstructionSet_AVXhas to be changed toInstructionSet_AVX2(a newInstructionSet_AVXwill be added). I am not sure if any other name of "AVX" should be changed to "AVX2" (e.g., canUseAVX), so we can discuss in this PR.