[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/Target/X86/X86ISelDAGToDAG.cpp:841
+        SDValue From[] = {SDValue(N, 0), SDValue(N, 1)};
+        SDValue To[] = {SDValue(Res.getNode(), 0), SDValue(Res.getNode(), 1)};
+        CurDAG->ReplaceAllUsesOfValuesWith(From, To, 2);
----------------
Use Res.getValue(0) and Res.getValue(1) to shorten the code.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1173
 
+    setOperationAction(ISD::STRICT_FP_TO_SINT,  MVT::v8i32, Legal);
+
----------------
Put this with its non-STRICT line


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1354
     setOperationPromotedToType(ISD::FP_TO_SINT, MVT::v4i1,  MVT::v4i32);
     setOperationPromotedToType(ISD::FP_TO_UINT, MVT::v4i1,  MVT::v4i32);
     setOperationAction(ISD::FP_TO_SINT,         MVT::v2i1,  Custom);
----------------
Don't we need to promote STRICT_FP_TO_SINT/UINT here?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1360
+    setOperationAction(ISD::STRICT_FP_TO_UINT,  MVT::v2i1,  Custom);
     // There is no byte sized k-register load or store without AVX512DQ.
     if (!Subtarget.hasDQI()) {
----------------
Blank line here


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1433
     setOperationPromotedToType(ISD::FP_TO_SINT, MVT::v16i16, MVT::v16i32);
     setOperationPromotedToType(ISD::FP_TO_SINT, MVT::v16i8, MVT::v16i32);
     setOperationPromotedToType(ISD::FP_TO_SINT, MVT::v16i1, MVT::v16i32);
----------------
Need to prmote the strict notes too


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1441
     setOperationAction(ISD::UINT_TO_FP,         MVT::v16i32, Legal);
+    setOperationAction(ISD::STRICT_FP_TO_SINT,  MVT::v16i32, Legal);
+    setOperationAction(ISD::STRICT_FP_TO_UINT,  MVT::v16i32, Legal);
----------------
Put these next to their equivalent non-STRICT lines.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19651
       MVT TruncVT = MVT::v4i1;
       unsigned Opc = IsSigned ? X86ISD::CVTTP2SI : X86ISD::CVTTP2UI;
       if (!IsSigned && !Subtarget.hasVLX()) {
----------------
Since we removed the fallthrough out of the if, just move this down to where its used.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19656
         TruncVT = MVT::v8i1;
-        Opc = ISD::FP_TO_UINT;
+        Opc = IsStrict ? ISD::STRICT_FP_TO_UINT : ISD::FP_TO_UINT;
         Src = DAG.getNode(ISD::INSERT_SUBVECTOR, dl, MVT::v8f64,
----------------
Make this a local variable to this block so you can move the earlier declaration below this if


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19669
+          Res = DAG.getMergeValues({Res, Chain}, dl);
+        return DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, MVT::v2i1, Res,
+                           DAG.getIntPtrConstant(0, dl));
----------------
If the IsStrict is taken, then this does an extract_subvector from a merge values and then deosn't return the chain. Are we missing test coverage? I think that should have triggered an assert.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19696
+      if (IsStrict)
+        return DAG.getMergeValues({Res, Chain}, dl);
+      return Res;
----------------
Just move this into the earlier IsStrict if. No need for back to back ifs with the same condition


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:28554
+          SDNodeFlags Flags;
+          Flags.setFPExcept(true);
+          Res->setFlags(Flags);
----------------
Please add a FIXME here, setting this to true unconditionally will need to be fixed when we stop losing flags.


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

https://reviews.llvm.org/D71592