[PATCH] D69275: Add constrained int->FP intrinsics

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

kpn marked 6 inline comments as done.
kpn added a comment.

I'm still working on this ticket daily! I'm trying to merge the two vector unrolling functions like Ulrich suggested. But I ran into problems that lead me to think we may have a serious issue lurking that we'll need to fix. That's what I've been working on: trying to understand the issue and see if it needs further investigation.

If you are in a hurry then you could have sent me an email and I would have uploaded the diffs I've got without further investigation.

I'm leaving attached the comments on my work that I've been adding but haven't submitted until now.



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:4125
   /// \param N Node to expand
   /// \param Result output after conversion
   /// \returns True, if the expansion was successful, false otherwise
----------------
uweigand wrote:
> Should document Chain here.
> 
> Or maybe it would be better to use MERGE_VALUES to return both components if appropriate?
Documentation. Check. I'll have that in my next round.

I don't remember why the prototype for expandFP_TO_UINT() got reformatted. But this code is already in the tree. Having expandUINT_TO_FP() be consistent with the existing tree seems like a good idea?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:176
+  SDValue ExpandLegalINT_TO_FP(bool isSigned, SDNode *Node, EVT DestVT,
                                const SDLoc &dl);
   SDValue PromoteLegalINT_TO_FP(SDValue LegalOp, EVT DestVT, bool isSigned,
----------------
uweigand wrote:
> If we pass Node, then all other arguments are really redundant, aren't they?
Seems that way. I'll give it a try.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:178
   SDValue PromoteLegalINT_TO_FP(SDValue LegalOp, EVT DestVT, bool isSigned,
                                 const SDLoc &dl);
   void PromoteLegalFP_TO_INT(SDNode *N, const SDLoc &dl,
----------------
uweigand wrote:
> Hmm, wouldn't we also need to update this routine?  Or can we say that promotion is not appropriate for handling strict semantics anyway?
I don't have a test for it so I didn't change it.

Can we guarantee the result would be rounded back down? Seems like promotion would be invalid without that guarantee.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2949
+        // Replace the new UINT result.
+        ReplaceNodeWithValue(SDValue(Node, 0), Tmp1);
+        LLVM_DEBUG(dbgs() << "Successfully expanded STRICT_UINT_TO_FP node\n");
----------------
uweigand wrote:
> Why do we have to do the Replace... thing here instead of just appending both to Results (like is done for other nodes with chain)?
I've had a lot of trouble with this, but at the moment I'm unable to reproduce any issue here. Let's simplify and see how it goes.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:329
         TLI.getStrictFPOperationAction(Node->getOpcode(),
                                    Node->getValueType(0))
         == TargetLowering::Legal) {
----------------
uweigand wrote:
> This should now also use ValVT, I think.
Looks like it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7349
+    if (N1.getValueType() == VT)
+      return N1; // noop conversion.
+    break;
----------------
craig.topper wrote:
> kpn wrote:
> > craig.topper wrote:
> > > This only works if N2 is result 1 of N1.
> > I don't understand. N2 is a constant that is either 0 or 1. What will happen if it is discarded here?
> > 
> > This code was lifted straight out of getNode() somewhere around line 5194. Without it the X86 target dies trying to lower a rounding of f64 to f64. This happens because getStrictFPExtendOrRound() returns a round when input and output are the same size. This mirrors the non-strict getFPExtendOrRound().
> Oops you’re right about N2. But there is an issue with the chain. Which I think is what I was thinking N2 was when I wrote that. If the input chain didn’t come from N1 this is broken.
> 
> I don’t see any precedent for returning a MERGE_VALUES from getNode. I think we need to fix the caller of getStrictFPExtendOrRound to only call when necessary.
Done. I've also added an assert to document that getStrictFPExtendOrRound() wants the lengths to be different.


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

https://reviews.llvm.org/D69275