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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a subscriber: RKSimon.
craig.topper added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4828
+      // Relink the chain
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Result.getValue(1));
+    }
----------------
kpn wrote:
> 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.
Most of those are in the type legalizer though aren't they? I guess my concern here is that this is a helper function that can be called from multiple places. Doing part of the replacement inside make the behavior of this function confusing.


================
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);
----------------
kpn wrote:
> 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?
@RKSimon do you have an opinion here?


================
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)
----------------
kpn wrote:
> 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.
Ok. Should we file bugs to implement support in the other targets?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63782