[PATCH] D132641: [X86] Support SAE for VCVTPS2PH from intrinsic.

[Freddy, Ye via Phabricator via llvm-commits]

FreddyYe added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27151-27152
       if (isAllOnesConstant(Mask))
-        return DAG.getNode(IntrData->Opc0, dl, Op.getValueType(), Src, Rnd);
+        return DAG.getNode(SAE ? X86ISD::CVTPS2PH_SAE : IntrData->Opc0, dl,
+                           Op.getValueType(), Src, Rnd);
 
----------------
pengfei wrote:
> We need to clear the SAE bit by replace the imm with the new `RC`.
I was to save code changes to implement, since hardware will ignore except the lower three bits.


================
Comment at: llvm/test/CodeGen/X86/avx512-intrinsics.ll:1013-1014
 ; X64:       # %bb.0:
-; X64-NEXT:    kmovw %edi, %k1
-; X64-NEXT:    vcvtps2ph $2, %zmm0, %ymm2 {%k1} {z}
-; X64-NEXT:    vcvtps2ph $2, %zmm0, %ymm1 {%k1}
-; X64-NEXT:    vpaddw %ymm1, %ymm2, %ymm1
+; X64-NEXT:    vcvtps2ph $11, {sae}, %zmm0, %ymm1
+; X64-NEXT:    vcvtps2ph $12, {sae}, %zmm0, %ymm2
+; X64-NEXT:    vpaddw %ymm2, %ymm1, %ymm1
----------------
pengfei wrote:
> This is not correct. The `{%k1}`/`{%k1} {z}` are missing. The same below.
> 
> The root reason is its intrinsic data is different from other mask ones: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86IntrinsicsInfo.h#L788-L793
> 
> Maybe we can consider define a new `INTR_TYPE_1OP_IMM8_MASK_SAE`?
Sorry didn't notice this fault. I would say to extend `MCVTPS2PH` to `MCVTPS2PH_SAE`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132641/new/

https://reviews.llvm.org/D132641