[PATCH] D43515: More math intrinsics for conservative math handling

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: docs/LangRef.rst:13260
+      declare <type>
+      @llvm.experimental.constrained.round(<type> <op>,
+                                          metadata <rounding mode>,
----------------
kpn wrote:
> andrew.w.kaylor wrote:
> > kpn wrote:
> > > andrew.w.kaylor wrote:
> > > > This intrinsic is a bit troublesome. The llvm.round intrinsic says that it "returns the same values as the libm round functions would, and handles error conditions in the same way." The libm round function, in turn, is documented as rounding to the nearest integer (and away from zero in halfway cases) regardless of the current rounding mode.
> > > > 
> > > > So what do we want the constrained form of the intrinsic to do? I think it needs to ignore the rounding mode. I'm not sure about exception behavior. If it doesn't respect exception behavior then we probably don't want to have the constrained form of this intrinsic at all.
> > > We'll still need the constrained intrinsic to avoid getting reordered.
> > > 
> > > How about if I rename the intrinsic to be fptrunc instead of round? Then the rounding would be explicit.
> > No, fptrunc does something else, right? My concern is that if this is preserving the behavior of the round library function (and I think we need to) then the rounding mode argument isn't relevant, so maybe it should be omitted in this case. The key thing is to be explicit about its behavior in the documentation and then make sure the implementation actually does what we say it will.
> Well, if I'm reading  SelectionDAGBuilder::visitFPTrunc() correctly then fptrunc gets turned into ISD::FP_ROUND. So, no, renaming this intrinsic to be fptrunc would not be wrong. Contrast this with llvm.round getting turned into ISD::FROUND. I think we need constrained intrinsics for both, so this one in this patch should be renamed after fptrunc.
> 
> I haven't touched the ISD::FROUND node yet, and that's what llvm.round gets turned into from the looks of SelectionDAGBuilder::visitIntrinsicCall(). That could be a later patch.
> 
> I agree that the rounding mode should be ignored and shouldn't be in the intrinsic's metadata.
> 
> WDYT?
You shouldn't be thinking in terms of the SelectionDAG at this level. ISD::FP_ROUND is very poorly named and does not do the same thing that llvm.round does. ISD::FP_ROUND converts a floating point number to a smaller type, but not necessarily an integer. That's fptrunc.

I suppose you are right that we do need a constrained version of fptrunc. I'm a bit concerned by the things that the LLVM language definition says are undefined. Those are the cases that will be of most interest for the constrained case and we should document the expected behavior, but I think we need to consider why the current spec says the result is undefined.

In any event llvm.round, which is what I thought you were replacing here, is something completely different.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:968
     case ISD::STRICT_FNEARBYINT: EqOpc = ISD::FNEARBYINT; break;
+    case ISD::STRICT_FP_TO_SINT: EqOpc = ISD::FP_TO_SINT; break;
+    case ISD::STRICT_FP_TO_UINT: EqOpc = ISD::FP_TO_UINT; break;
----------------
kpn wrote:
> andrew.w.kaylor wrote:
> > kpn wrote:
> > > andrew.w.kaylor wrote:
> > > > STRICT_FP_TO_SINT needs to do something more than this. The default lowering of FP_TO_SINT is known to raise spurious FE_INEXACT exceptions because it involves speculative execution.
> > > I have seen FP_TO_SINT cause traps when it shouldn't. Would having that default lowering use the chain in the STRICT_ case solve that issue?
> > No, I don't think the chain will fix this. We need to implement strict lowering that does something different.
> Is there something we can do at this level to fix this? If there is then I'm all for it, but if there isn't then we should probably still put the intrinsic in. 
> 
> We'll need it eventually, and currently none of the constrained intrinsics solve the complete optimization problem. So I wonder if this intrinsic is really all that different from the other experimental constrained intrinsics.
> 
> If a backend models FP side effects then wouldn't the existing default lowering work correctly?
To be honest I'm not entirely sure how to fix this in the current selection DAG model. The issue is that we need to introduce a branch to fix the problem, but by the time we're selecting instructions it's too late to do that. I think it needs to be addressed when we're building the DAG,


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6134
   ComputeValueVTs(TLI, DAG.getDataLayout(), FPI.getType(), ValueVTs);
-  ValueVTs.push_back(MVT::Other); // Out chain
+  if (!(Opcode == ISD::STRICT_FP_TO_UINT || 
+        Opcode == ISD::STRICT_FP_TO_SINT ||
----------------
kpn wrote:
> andrew.w.kaylor wrote:
> > kpn wrote:
> > > andrew.w.kaylor wrote:
> > > > Why are you not attaching these nodes to the chain?
> > > Because they need to match what the default lowering is expecting. Otherwise a variety of failures happen.
> > There is code that removes the chain when the strict node is mutated to a non-strict node. That should be preventing lowering problems. I believe the chain was necessary to prevent re-ordering prior to final instruction selection.
> Mutation happens too late in some cases. 
> 
> If it happened early enough then there wouldn't be any need for the strict intrinsics to be mentioned in SelectionDAGLegalize::ExpandNode(). Since it doesn't we can't use the chain.
> 
> Should that mutation happen earlier?
We need the mutation to be put off as long as possible. I think it should be possible to unlink the chain in ExpandNode if necessary. Depending on what's happening there, we might even want the expanded node to make use of the chain.


Repository:
  rL LLVM

https://reviews.llvm.org/D43515