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

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

kpn added inline comments.


================
Comment at: docs/LangRef.rst:13213
+
+The result produced is an unsigned integer converted from the floating
+point operand.
----------------
andrew.w.kaylor wrote:
> I think you need to say more about the semantics. The LLVM IR fptoui and fptosi are specifically documented as rounding to zero. I don't think we want that with the constrained intrinsics so we need to very specifically document how they will be different from the standard instructions.
What do we want these intrinsics to do that is different from the normal IR fptoui and fptosi?

I don't think any of the constrained intrinsics _today_ are doing anything with the rounding and exception metadata. That makes it hard for me to say much about it in documentation, today.

Well, unless I misunderstood the current code. That's totally possible.


================
Comment at: docs/LangRef.rst:13260
+      declare <type>
+      @llvm.experimental.constrained.round(<type> <op>,
+                                          metadata <rounding mode>,
----------------
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.


================
Comment at: docs/LangRef.rst:13296
+      declare <type>
+      @llvm.experimental.constrained.round.inreg(<type> <op1>,
+                                          <type> <op2>,
----------------
andrew.w.kaylor wrote:
> This seems to be leaking SelectionDAG implementation details into the IR space. How is this used?
It seems to exist for the MVT::ppcf128 type. A quick grep doesn't show any other users. 

Test coverage is lacking. I added a test using the intrinsic, but I had to mark it expected fail since I couldn't get it to work.

To avoid the risk of bugs getting introduced later I went ahead and implemented the intrinsic. Would it be better to not have the intrinsic and to instead have a pass that replaces the non-STRICT SDNode with a STRICT version? That would avoid said leaking into the IR space. It would, however, mean that llvm would have an opinion on when STRICT nodes should be used. I'm not sure that's a good thing. 


================
Comment at: docs/LangRef.rst:13337
+      declare <type>
+      @llvm.experimental.constrained.extend(<type> <op>,
+                                          metadata <rounding mode>,
----------------
andrew.w.kaylor wrote:
> This is a replacement for fpext, right? I think you should say that somewhere.
I think I picked names for the intrinsics that matched the SelectionDAG node enum. Perhaps it would be better to match the bitcode language names? In which case this intrinsic would be "fpext" instead of "extend".

Either way that's a good idea for the documentation to at least mention fpext.


================
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;
----------------
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?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1152
     // ISD::STRICT_FSQRT.
-    Action = getStrictFPOpcodeAction(TLI, Node->getOpcode(),
-                                     Node->getValueType(0));
+    if (Node->getOpcode() == ISD::STRICT_FP_ROUND_INREG) {
+      EVT InnerType = cast<VTSDNode>(Node->getOperand(1))->getVT();
----------------
andrew.w.kaylor wrote:
> Since this gets unique handling why isn't it just a separate case from the others?
Good point. And making it a separate case also takes care of the formatting issue in the else block.


================
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 ||
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D43515