[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:13213
+
+The result produced is an unsigned integer converted from the floating
+point operand.
----------------
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.


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


================
Comment at: docs/LangRef.rst:13296
+      declare <type>
+      @llvm.experimental.constrained.round.inreg(<type> <op1>,
+                                          <type> <op2>,
----------------
This seems to be leaking SelectionDAG implementation details into the IR space. How is this used?


================
Comment at: docs/LangRef.rst:13337
+      declare <type>
+      @llvm.experimental.constrained.extend(<type> <op>,
+                                          metadata <rounding mode>,
----------------
This is a replacement for fpext, right? I think you should say that somewhere.


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


================
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();
----------------
Since this gets unique handling why isn't it just a separate case from the others?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1157
+    }
+    else Action = getStrictFPOpcodeAction(TLI, Node->getOpcode(),
+                                       Node->getValueType(0));
----------------
The style/formatting is wrong here. I think you need curly braces around your else-clause and the "else" itself needs to be on the same line as the curly brace above it.


================
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 ||
----------------
Why are you not attaching these nodes to the chain?


Repository:
  rL LLVM

https://reviews.llvm.org/D43515