[PATCH] D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

This doesn't look correct.  As far as I can see, none of the conversion functions were actually changed to handle strict operations.  For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.

The patch only adds a strict variant of the direct move, which seems to me the only operation where actually a strict version is **not** required ...



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8230
+      Src = DAG.getNode(ISD::STRICT_FP_EXTEND, dl, {MVT::f64, MVT::Other},
+                        {Op.getOperand(0), Src});
+    else
----------------
This doesn't look right.   The Chain produced by this strict node just vanishes, this cannot be correct.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8247
     Conv = DAG.getNode(IsSigned ? PPCISD::FCTIDZ : PPCISD::FCTIDUZ, dl,
                        MVT::f64, Src);
   }
----------------
And you need strict versions of all these conversion operations, I'd assume.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8258
+                  Op.getOpcode() == ISD::STRICT_FP_TO_SINT;
 
   // Convert the FP value to an int value through memory.
----------------
Nothing in the function actually handles strict nodes, that cannot be right.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8307
+    return DAG.getNode(PPCISD::MFVSR, dl, Op.getValueType(),
+                       convertFPToInt(Op, DAG, Subtarget));
 }
----------------
Why do we need a strict version of a plain move?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8312
                                           const SDLoc &dl) const {
-  SDValue Src = Op.getOperand(0);
+  SDValue Src = Op.getOperand(Op->isStrictFPOpcode() ? 1 : 0);
   // FP to INT conversions are legal for f128.
----------------
Again, nothing in this function actually handles strict nodes ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537