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

[Kewen Lin via Phabricator via llvm-commits]

jedilyn added a comment.

Thanks a lot for your time @nemanjai , such a good idea to use script to verify various combination. I'm sorry I can't actually get your point about simplifying the logic in the switch block. Each case check in the switch seems not too much and exactly follow the patterns in above comments nearing the case, people can easily map the checks to patterns there. Do I miss something there? Could you kindly clarify more on that? Thanks in advance!



================
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()))
----------------
nemanjai wrote:
> Please explain why we will select an isel if this condition is satisfied.
OK, will add more comments here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4157
+    if (SelCCTVal == -1 && SelCCFVal == 1) {
+      std::swap(InnerLHS, InnerRHS);
+    } else if (SelCCTVal != 1 || SelCCFVal != -1)
----------------
nemanjai wrote:
> It is not clear to me why we can just swap these without changing the condition code.
This code is only for having internal select_cc, the ir looks like:
`  i64 = select_cc t2, t4, Constant:i64<-1>, Constant:i64<1>, setlt:ch`
Line 4126-4127 already ensure the outside select_cc uses seteq and true value zero.
So inner select_cc can be represented as:
`  (t2 < t4)? -1: 1; `

Since we want to canonicalize all innerSel to have true value 1 and false value -1, it actually implicitly swap values 1 and -1, so we only need to explictly swaps two comparision operands t2 and t4. 
After the swapping it's changed to:
`  (t4 < t2)? 1: -1`
It's already equivelant to the original expression, we don't need to swap 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))
----------------
nemanjai wrote:
> 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.?
Sorry, I can't really catch your point here. The condition expression is to check all expected pattern listed in above close comments, they should be consistent. I'm not clear on what is difficult to follow? The condition combination or the `NeedSwapOps `computation? 


================
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;
----------------
nemanjai wrote:
> As a matter of custom, we put these into `PPCInstrInfo.td`.
I was originally intended to move SETB to `PPCInstrInfo.td`, but I found this instruction is flagged as PPC64 only. I searched around in `PPCInstrInfo.td` and can't find any similar cases, so I didn't think it's a good idea to move one 64bit only instruction out of `PPCInstr64Bit.td`.


Repository:
  rL LLVM

https://reviews.llvm.org/D53275