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

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

kpn marked 5 inline comments as done.
kpn added inline comments.


================
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,
----------------
andrew.w.kaylor wrote:
> 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.
Eh, I was just lumping the rounding nodes together. It's not important. Avoiding pain downstream overwhelms that weak reason. I'll change it.


================
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 "
----------------
andrew.w.kaylor wrote:
> Why are you changing this assertion?
Because it is stronger than SelectionDAG::ReplaceAllUsesWith(SDNode *From, SDNode *To) needs it to be (unless I'm wrong), and because without this change the test case cannot pass. 

The conversion to a libcall results in a node with result value, a chain, and glue. There was no glue before, so the added result would trigger the assert.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2045
     EVT ArgVT = Op.getValueType();
+    if (ArgVT.isSimple() && ArgVT.getSimpleVT() == MVT::Other) {
+      CurInChain = Op;
----------------
andrew.w.kaylor wrote:
> 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.
So we don't need the chain to preserve ordering since it's a function call? That makes things simpler. Here, at least. I don't know what happens in ReplaceNode when a Chain result gets swapped out with a Glue result.


================
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
----------------
andrew.w.kaylor wrote:
> If these changes do need to stay, you'll need to update this comment.
I'd love to see a test case for this. You'll notice the assert() I added below because I don't have one. That would make it much easier to update the comment. it does need to be updated.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2940
+                                         RTLIB::LROUND_F128,
+                                         RTLIB::LROUND_PPCF128);
+    ReplaceNode(Node, Tmp1.getNode());
----------------
andrew.w.kaylor wrote:
> Shouldn't something be getting pushed to Results here?
No. If we go that route then we end up in ReplaceNode(SDNode *Old, const SDValue *New) with multiple SDValues to replace but only one of them being given. This results in ReplaceNode() running off into some other memory and typically crashing. We need to instead call ReplaceNode(SDNode *Old, SDNode *New) so all values that need to be looked at are present.

That's why I call ReplaceNode() myself here, and since we're bypassing the rest of the function I mostly-duplicated the debug message and the return statement.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64746