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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
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
----------------
Should document Chain here.

Or maybe it would be better to use MERGE_VALUES to return both components if appropriate?


================
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,
----------------
If we pass Node, then all other arguments are really redundant, aren't they?


================
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,
----------------
Hmm, wouldn't we also need to update this routine?  Or can we say that promotion is not appropriate for handling strict semantics anyway?


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


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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9156
 
 SDValue SelectionDAG::UnrollVectorOp(SDNode *N, unsigned ResNE) {
+  assert((N->getNumValues() == 1 ||
----------------
Hmm.   There's an UnrollVectorOp_StrictFP in LegalizeVectorTypes.cpp.  If we change **this** routine to also handle some strict operations, maybe we should go all the way and just have merge the routines completely?


================
Comment at: llvm/lib/IR/Verifier.cpp:4791
+    Assert((NumOperands == 3), "invalid arguments for constrained FP intrinsic",
+           &FPI);
+    Value *Operand = FPI.getArgOperand(0);
----------------
I believe the NumOperands check is now redunant with common tests handled above.


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

https://reviews.llvm.org/D69275