[PATCH] D71879: [X86] Custom widen 128/256-bit vXi32 fp_to_uint on avx512f targets without avx512vl. Similar for vXi64 on avx512dq without avx512vl.

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 3 inline comments as done.
craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19811-19812
+    if (VT == MVT::v8i32 && SrcVT == MVT::v8f64) {
+      assert(!IsSigned && "Expected unsigned conversion!");
+      assert(Subtarget.useAVX512Regs() && "Requires avx512f");
+      return Op;
----------------
pengfei wrote:
> Why no signed, they are legal too.
v8i32 FP_TO_SINT has a Legal setOperationAction so it should never get here. Same reason the code below only widens unsigned.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19837-19839
+      } else {
+        Res = DAG.getNode(ISD::FP_TO_UINT, dl, ResVT, Src);
+      }
----------------
pengfei wrote:
> Remove curly braces.
I tend to prefer curly braces on else if they are also on the if.


================
Comment at: llvm/test/CodeGen/X86/vec-strict-fptoint-256.ll:1122
 ; AVX512DQ:       # %bb.0:
 ; AVX512DQ-NEXT:    vcvttps2dq %ymm0, %ymm0
 ; AVX512DQ-NEXT:    vpmovdw %zmm0, %ymm0
----------------
pengfei wrote:
> There're tests for AVX512DQ that don't use zmm registers, I guess the actions for them are not correctly set.
vcvttps2dq ymm, ymm is an avx2 instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71879