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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:582
+    if (Subtarget.hasSPE()) {
+      setOperationAction(ISD::STRICT_FP_TO_UINT, MVT::i32, Legal);
       setOperationAction(ISD::FP_TO_UINT, MVT::i32, Legal);
----------------
Don’t do this for spe target and remove the test for spe. Sorry about the back and forth.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8158
+  assert(Chain && "Missing chain for creating strict nodes");
+  unsigned NewOpc = ISD::DELETED_NODE;
+  switch (Opc) {
----------------
You don't need this assertion now.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8203
            "i64 FP_TO_UINT is supported only with FPCVT");
-    Conv = DAG.getNode(Signed ? PPCISD::FCTIDZ : PPCISD::FCTIDUZ, dl, MVT::f64,
-                       Src);
+    Conv = DAG.getNode(IsSigned ? PPCISD::FCTIDZ : PPCISD::FCTIDUZ, dl,
+                       MVT::f64, Src);
----------------
So, do we have problem if it is strict opcode in this code path?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8261
+  SDValue Src = Op.getOperand(Op->isStrictFPOpcode() ? 1 : 0);
   assert(Src.getValueType().isFloatingPoint());
+  SDValue Chain;
----------------
move the assertion into concertFPToInt


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8428
 
 static SDValue convertIntToFP(SDValue Op, SDValue Src, SelectionDAG &DAG,
                               const PPCSubtarget &Subtarget) {
----------------
ConvertIntToFP and ConvertFPToInt should  have the same parameters. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8533
   SDValue Src = Op.getOperand(0);
-  bool Signed = Op.getOpcode() == ISD::SINT_TO_FP;
+  bool IsSigned = Op.getOpcode() == ISD::SINT_TO_FP;
 
----------------
Please remove such kind of change as it is not part of your change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537