[PATCH] D71592: [X86] Enable STRICT_FP_TO_SINT/UINT on X86 backend.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:612
+  if (IsStrict) {
+    Promoted = DAG.getNode(ISD::TRUNCATE, dl, VT, Promoted);
+    return DAG.getMergeValues({Promoted, Chain}, dl);
----------------
Move this above the if and make the non-STRICT path just return Promoted?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1442
+    setOperationPromotedToType(ISD::FP_TO_UINT,        MVT::v16i16, MVT::v16i32);
+    setOperationAction(ISD::SINT_TO_FP,                MVT::v16i32, Legal);
+    setOperationAction(ISD::UINT_TO_FP,                MVT::v16i32, Legal);
----------------
Move these two SINT_TO_FP/UINT_TO_FP lines below the STRICT lines. Better to keep the FP->INT lines stuff together.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1449
+    setOperationAction(ISD::STRICT_FP_TO_UINT,         MVT::v16i32, Legal);
+    setOperationPromotedToType(ISD::STRICT_FP_TO_UINT, MVT::v16i16, MVT::v16i32);
+    setOperationPromotedToType(ISD::STRICT_FP_TO_UINT, MVT::v16i8, MVT::v16i32);
----------------
Please keep these in the same order as their non-STRICT versions. Maybe put the STRICT line directly below its corresponding non-STRICT lines. We should make it easy to see that STRICT and non-STRICT are the same.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19674
+        }
+        else
+          Res = DAG.getNode(Opc, dl, ResVT, Src);
----------------
else should be at the end of the line above.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19690
       }
-      // FIXME: Strict fp!
-      assert(!IsStrict && "Unhandled strict operation!");
-      SDValue Res = DAG.getNode(Opc, dl, ResVT, Src);
+      else
+        Res = DAG.getNode(Opc, dl, {ResVT, MVT::Other},
----------------
Move else to the line above


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19711
+        return DAG.getMergeValues({Res, Chain}, dl);
+      } else
+        Res = DAG.getNode(Opc, dl, {VT, MVT::Other}, {DAG.getEntryNode(), Tmp});
----------------
Drop the else since we already returned in the if body.


================
Comment at: llvm/test/CodeGen/X86/vec-strict-fptoint-512.ll:3
+; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+avx512f -mattr=+avx512vl -O3 -disable-strictnode-mutation | FileCheck %s --check-prefixes=CHECK,AVX,AVX512-32
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512f -mattr=+avx512vl -O3 -disable-strictnode-mutation | FileCheck %s --check-prefixes=CHECK,AVX,AVX512-64
+
----------------
Should we have avx512dq coverage for the conversion to vXi64? Same with other vector tests.


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

https://reviews.llvm.org/D71592