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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added subscribers: jhibbits, chmeee.
nemanjai 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);
----------------
steven.zhang wrote:
> Don’t do this for spe target and remove the test for spe. Sorry about the back and forth.
I would defer to @jhibbits @chmeee (not sure which of Justin's ID's is active) regarding SPE bits.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8150
 
+static SDValue getFPNode(unsigned Opc, EVT VT, SDValue Op, SDValue Chain,
+                         SelectionDAG &DAG) {
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8153
+  SDLoc dl(Op);
+  if (!Chain)
+    return DAG.getNode(Opc, dl, VT, Op);
----------------
This seems dangerous to me. You are deciding whether to return a strict node based on whether a valid Chain is provided. I am personally against making decisions based on orthogonal concerns.
If the caller wants a strict node, that should be explicit rather than this strange implicit contract of "If you want a strict node, provide a valid chain."


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:443
+    /// Constrained direct move from VSR instruction.
+    STRICT_MFVSR = ISD::FIRST_TARGET_STRICTFP_OPCODE,
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537