[PATCH] D63782: [FPEnv] Add fptosi and fptoui constrained intrinsics

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:307
+    /// unsigned integer. These have the same semantics as fptosi and fptoui 
+    /// in IR. If the FP value cannot fit in the integer type, the results are
+    /// undefined.
----------------
Is this what we want from a strict implementation. "icc -fp-model=strict" goes out of its way to generate an invalid exception when the input type doesn't fit and the hardware can't generate an exception on its own. https://godbolt.org/z/ABU80i


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4828
+      // Relink the chain
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Result.getValue(1));
+    }
----------------
Shouldn't this be the caller's responsibility? It is for the data result.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4829
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Result.getValue(1));
+    }
+    else
----------------
Put else on the above line.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4838
 
-  bool Strict = shouldUseStrictFP_TO_INT(SrcVT, DstVT, /*IsSigned*/ false);
+  bool Strict = Node->isStrictFPOpcode() ||
+                shouldUseStrictFP_TO_INT(SrcVT, DstVT, /*IsSigned*/ false);
----------------
I think the use of "Strict" here is being overloaded. The "Strict" in shouldUseStrictFP_TO_INT isn't the same "strict" as the intrinsic. We may want the same behavior but we should clarify the terminology maybe.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4864
+      // Relink the chain
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), SInt.getValue(1));
+    }
----------------
Same question as above


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4865
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), SInt.getValue(1));
+    }
+    else
----------------
Put else on this line


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:293
+; CHECK-LABEL: @f20u
+; COMMON: cvttsd2si
+define i32 @f20u() {
----------------
I hope there are more instructions here. This is a signed conversion instruction. Or are we constant folding the setcc and select in the fp_to_uint expansion?


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:344
 declare double @llvm.experimental.constrained.fma.f64(double, double, double, metadata, metadata)
+declare i32 @llvm.experimental.constrained.fptosi.i32.f64(double, metadata)
+declare i32 @llvm.experimental.constrained.fptoui.i32.f64(double, metadata)
----------------
Where's the fptosi test case?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63782