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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn marked 6 inline comments as done.
kpn 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.
----------------
craig.topper wrote:
> 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
No, I don't think this is what we want. I'll remove that sentence.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4828
+      // Relink the chain
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Result.getValue(1));
+    }
----------------
craig.topper wrote:
> Shouldn't this be the caller's responsibility? It is for the data result.
That's a fair question. Splitting the responsibilities could be seen as confusing. But if you grep for "Legalize the chain result" you'll find 30+ places where we're already handling the chain the way we are here. So I think that we should be consistent here and if we want to change it everywhere then maybe another ticket would be better for that change.


================
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);
----------------
craig.topper wrote:
> 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.
Ok. How about UseOnlyOneFP_TO_SINT?


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:293
+; CHECK-LABEL: @f20u
+; COMMON: cvttsd2si
+define i32 @f20u() {
----------------
craig.topper wrote:
> 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?
It looks like it's constant folding away the rest of the instructions since 42 fits within the lowest bits of a value. I can change it to take a variable and then check for the other interesting instructions.


================
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)
----------------
craig.topper wrote:
> Where's the fptosi test case?
I had to move the tests over to the PowerPC's e500 chip with the SPE feature. That's the in-tree target that still has scalar FP_TO_SINT marked Legal. Most everywhere else it is not marked Legal and currently fails with the existing mutation support.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63782