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

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

kpn marked an inline comment as done and an inline comment as not done.
kpn added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4828
+      // Relink the chain
+      DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Result.getValue(1));
+    }
----------------
craig.topper wrote:
> 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.
OK, that's fair. I'll change it.


================
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:
> 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?
The problem is in TargetLoweringBase::getStrictFPOperationAction(). The target is queried to see how it handles the non-strict version of a strict node, and then any result that isn't Legal gets bashed into Expand. So Promote and Custom can't be handled. That's the problem that needs to be solved. 

Short term I'm not sure what to do. Maybe yank out the lines that bash Promote and Custom? That's not been tried and I don't know exactly what will happen.

Long term the mutation compatibility code needs to die.

Also, it was pointed out to me recently that the strict and non-strict nodes need to have the same action registered. Which is true. Do we want to put in checks to verify that this is the case at some point?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63782