[PATCH] D64746: Add constrained intrinsics for lrint and lround

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: docs/LangRef.rst:15705
+The '``llvm.experimental.constrained.lrint``' intrinsic returns the first
+operand rounded to the nearest integer. It may raise an inexact floating-point
+exception if the operand is not an integer.
----------------
I know you're just copying what we previously said about the constrained rint, but we should probably say that it "will" raise this exception.


================
Comment at: docs/LangRef.rst:15724
+mode is determined by the runtime floating-point environment.  The rounding
+mode argument is only intended as information to the compiler.
+
----------------
We should describe what is returned if the value is too large to be represented as a long. The llvm.lrint doesn't do that either, but it should too.


================
Comment at: docs/LangRef.rst:15736
+      declare <inttype>
+      @llvm.experimental.constrained.llrint(<fptype> <op1>,
+                                            metadata <rounding mode>,
----------------
It seems like we have a problem here. The intrinsics are overloaded to take any integer as a return type, but not all integers will match up with a real library call. Is that handled anywhere?


================
Comment at: docs/LangRef.rst:16010
+The first argument is a floating-point number. The return value is an
+integer type.
+
----------------
Can you describe the rounding mode that will be used? You should also describe the conditions under which an exception will be raised.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:303
     STRICT_FRINT, STRICT_FNEARBYINT, STRICT_FMAXNUM, STRICT_FMINNUM,
-    STRICT_FCEIL, STRICT_FFLOOR, STRICT_FROUND, STRICT_FTRUNC,
+    STRICT_FCEIL, STRICT_FFLOOR, STRICT_LROUND, STRICT_LLROUND, STRICT_FROUND,
+    STRICT_FTRUNC, STRICT_LRINT, STRICT_LLRINT,
----------------
What's the logic behind the ordering here? If there's no good reason to insert new nodes in the middle of the existing list, it would be kinder to people maintaining out-of-tree branches if you would append them to the end of this group.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:208
 
-    assert(Old->getNumValues() == New->getNumValues() &&
+    assert(Old->getNumValues() <= New->getNumValues() &&
            "Replacing one node with another that produces a different number "
----------------
Why are you changing this assertion?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2045
     EVT ArgVT = Op.getValueType();
+    if (ArgVT.isSimple() && ArgVT.getSimpleVT() == MVT::Other) {
+      CurInChain = Op;
----------------
It's not clear to me why this is necessary now but hasn't been before. Since we're expanding to a library call, I don't think we need to preserve the chain that the strict FP node was using.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2062
 
   // By default, the input chain to this libcall is the entry node of the
   // function. If the libcall is going to be emitted as a tail call then
----------------
If these changes do need to stay, you'll need to update this comment.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2940
+                                         RTLIB::LROUND_F128,
+                                         RTLIB::LROUND_PPCF128);
+    ReplaceNode(Node, Tmp1.getNode());
----------------
Shouldn't something be getting pushed to Results here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64746