[PATCH] D53275: [Power9] Exploit power9 new instruction setb

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Since the logic in the switch statement is difficult to follow, I ended up having to test this patch with a script that tries all combinations of comparisons, results and operand order in order to convince myself that it doesn't emit a `setb` when it is not valid to do so. Everything checks out there, but I don't really have an easy way of testing whether any opportunities are missed.
I am really hoping that you can simplify the logic in that switch to make it easier to follow.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:85
+STATISTIC(NumP9Setb,
+          "Number of exploiting Power9 setb instruction times.");
 
----------------
Perhaps `Number of compares lowered to setb.`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4136
+
+  // setb has longer latency than isel, avoid to replace isel.
+  if (!SetOrSelCC.hasOneUse() || (!InnerIsSel && !FalseRes.hasOneUse()))
----------------
Please explain why we will select an isel if this condition is satisfied.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4157
+    if (SelCCTVal == -1 && SelCCFVal == 1) {
+      std::swap(InnerLHS, InnerRHS);
+    } else if (SelCCTVal != 1 || SelCCFVal != -1)
----------------
It is not clear to me why we can just swap these without changing the condition code.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4197
+  case ISD::SETLT:
+    if (InnerCC == ISD::SETNE || (InnerCC == ISD::SETGT && !InnerSwapped) ||
+        (InnerCC == ISD::SETLT && InnerSwapped))
----------------
I think expressions such as this are extremely difficult to follow. Perhaps the expressions for computing the Boolean output parameters can be simplified using the fact that unsigned comparisons have the property `CC & 0x8 != 0`, etc.?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:776
                        "maddld $RT, $RA, $RB, $RC", IIC_IntMulHD, []>, isPPC64;
-def SETB : XForm_44<31, 128, (outs g8rc:$RT), (ins crrc:$BFA),
-                     "setb $RT, $BFA", IIC_IntGeneral>, isPPC64;
+def SETB : XForm_44<31, 128, (outs gprc:$RT), (ins crrc:$BFA),
+                     "setb $RT, $BFA", IIC_IntGeneral>, isPPC64;
----------------
As a matter of custom, we put these into `PPCInstrInfo.td`.


Repository:
  rL LLVM

https://reviews.llvm.org/D53275