[PATCH] D71871: [X86] Enable STRICT_SINT_TO_FP/STRICT_UINT_TO_FP on X86 backend

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:568
+                     : ISD::SIGN_EXTEND;
   for (unsigned j = 0; j != Op.getNumOperands(); ++j) {
+    if (IsStrict && j == 0) {
----------------
Is a for loop needed here? Isn’t there only one non chain operand?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:571
+      Operands[j] = Op.getOperand(0);
+      ++j;
+    }
----------------
If the loop is needed I’d use a continue instead of incrementing j. Or handle it outside the loop and start the loop at 1 for strict.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7390
+           // FIXME: I guess this should be bitsLE just like normal FP_ROUND.
+           VTList.VTs[0].bitsLE(Ops[1].getValueType()) &&
            isa<ConstantSDNode>(Ops[2]) &&
----------------
Do not change this. Normal FP_ROUND has a special case for equal types to skip creating the node. We can’t do that for strict because we can’t manage the chain here. The caller needs to avoid calling getNode when the types are equal.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18896
+                              {MVT::v2f64, MVT::Other}, {Op.getOperand(0), HI});
+    fHI = DAG.getNode(ISD::STRICT_FMUL, DL, {MVT::v2f64, MVT::Other},
+                      {SDValue(fHI.getNode(), 1), fHI, TWOHW});
----------------
The chain result from this isn’t used. You probably need a token factor somewhere


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18897
+    fHI = DAG.getNode(ISD::STRICT_FMUL, DL, {MVT::v2f64, MVT::Other},
+                      {SDValue(fHI.getNode(), 1), fHI, TWOHW});
+    SDValue fLO = DAG.getNode(X86ISD::STRICT_CVTSI2P, DL,
----------------
Use getValue


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18905
+                                 {SDValue(fLO.getNode(), 1), fHI, fLO});
+    DAG.ReplaceAllUsesOfValueWith(Op.getValue(1), SDValue(Result.getNode(), 1));
+    return Result;
----------------
Use merge values don’t do your own replacement


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19009
+    //     return (float4) lo + fhi;
+    SDValue LowBitcast = DAG.getBitcast(VecFloatVT, Low);
+    SDValue Result = DAG.getNode(ISD::STRICT_FADD, DL, {VecFloatVT, MVT::Other},
----------------
Can the low bit cast be moved up and shared by both paths?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19012
+                                 {SDValue(FHigh.getNode(), 1), LowBitcast,
+                                  FHigh});
+    DAG.ReplaceAllUsesOfValueWith(Op.getValue(1), SDValue(Result.getNode(), 1));
----------------
getValue


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19013
+                                  FHigh});
+    DAG.ReplaceAllUsesOfValueWith(Op.getValue(1), SDValue(Result.getNode(), 1));
+    return Result;
----------------
Merge values


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:43804
+    Chain = N->getOperand(0);
+  if (SDValue Res = combineVectorCompareAndMaskUnaryOp(N, DAG, Chain))
     return Res;
----------------
The chain result isn’t being returned or connected


================
Comment at: llvm/test/CodeGen/X86/fp-intrinsics.ll:2204
 ; X87:       # %bb.0: # %entry
-; X87-NEXT:    subl $12, %esp
-; X87-NEXT:    .cfi_def_cfa_offset 16
----------------
What change caused these test changes? It looks like most of this patch is vector related. What did I miss?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71871