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

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf marked 9 inline comments as done.
qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8150
 
+static SDValue getFPNode(unsigned Opc, EVT VT, SDValue Op, SDValue Chain,
+                         SelectionDAG &DAG) {
----------------
nemanjai wrote:
> This function appears to just take a node and produce a strict version of that node. It seems like there isn't anything target dependent about such an operation so it is very suspicious to me why this is in the PPC back end.
> If for some reason it has to be here, please explain why in a comment. If the only target specific part of this is the `STRICT_MFVSR` node, then at least the rest can be handled by target independent code, can't it?
Thanks for the comments. The method (including some overloads) doesn't help much. So I removed it and wrote a simple helper method to get strict version of ppc-specific opcode.

There's method `mutateStrictFPToFP` doing similar things but maybe not suitable here. I'll send an NFC to make opcode conversion a hook so that each target can benefit from it.


================
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);
----------------
steven.zhang wrote:
> So, do we have problem if it is strict opcode in this code path?
Do you mean these PPC-specific opcodes are not strict? But the result is either load/store or direct moved. What we do here is to keep operands of value consistent. So changing these opcodes to strict may be unnecessary.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8428
 
 static SDValue convertIntToFP(SDValue Op, SDValue Src, SelectionDAG &DAG,
                               const PPCSubtarget &Subtarget) {
----------------
steven.zhang wrote:
> ConvertIntToFP and ConvertFPToInt should  have the same parameters. 
FPToInt is round-then-move, while IntToFP is move-then-round. So when IntToFP we need extra information from original `Op` besides the moved `Src`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:443
+    /// Constrained direct move from VSR instruction.
+    STRICT_MFVSR = ISD::FIRST_TARGET_STRICTFP_OPCODE,
+
----------------
nemanjai wrote:
> Why? The instruction simply moves bits around. It does not cause any exceptions, it is not subject to rounding, etc. If this is necessary, it needs to be clear from the comment why.
Because (1) this prevents it being combined somewhere unexpectedly; (2) all strict nodes have extra operand for their chains, so replacing original `strict_*` node with non-strict one will cause operands mismatch. I added necessary comments. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537