This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Exploit power9 new instruction setb
ClosedPublic

Authored by jedilyn on Oct 15 2018, 2:39 AM.

Details

Summary
; Function Attrs: norecurse nounwind readnone
define i64 @_Z4setbxx(i64, i64) local_unnamed_addr #0 {
  %3 = icmp slt i64 %0, %1
  %4 = icmp ne i64 %0, %1
  %5 = zext i1 %4 to i64
  %6 = select i1 %3, i64 -1, i64 %5
  ret i64 %6
}

Current seq: (11 cycs)

xor 6, 3, 4
li 5, -1
addic 7, 6, -1
cmpd    3, 4
subfe 6, 7, 6
isel 3, 5, 6, 0
blr

Expected seq: (8 cycs)

cmpd    3, 4
setb 3, 0
blr

Diff Detail

Repository
rL LLVM

Event Timeline

jedilyn created this revision.Oct 15 2018, 2:39 AM

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.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
85 ↗(On Diff #169664)

Perhaps Number of compares lowered to setb.

4136 ↗(On Diff #169664)

Please explain why we will select an isel if this condition is satisfied.

4157 ↗(On Diff #169664)

It is not clear to me why we can just swap these without changing the condition code.

4197 ↗(On Diff #169664)

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.?

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
776 ↗(On Diff #169664)

As a matter of custom, we put these into PPCInstrInfo.td.

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!

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4136 ↗(On Diff #169664)

OK, will add more comments here.

4157 ↗(On Diff #169664)

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.

4197 ↗(On Diff #169664)

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?

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
776 ↗(On Diff #169664)

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.

jedilyn updated this revision to Diff 171628.Oct 29 2018, 7:53 PM

Addressed some comments from Nemanja.

jedilyn marked 4 inline comments as done.Oct 29 2018, 7:55 PM
jedilyn added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
776 ↗(On Diff #169664)

Does it make sense to still keep it in PPCInstr64Bit.td?

jedilyn added inline comments.Oct 29 2018, 8:11 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
85 ↗(On Diff #171628)

Need one more period "." at the end , will fix it with any possible updates for below unresolved comments.

gentle ping...

gentle ping...

gentle ping.... this is a patch for Power9 setb exploitation

jsji added a comment.Dec 13 2018, 10:39 AM

Mostly good to me except some minor changes. @nemanjai Do you have any further comments regarding @jedilyn 's latest update? Thanks.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4710 ↗(On Diff #171628)

Can we explain more why we need to avoid seteq optimization in SelectCC?

llvm/test/CodeGen/PowerPC/ppc64-P9-setb.ll
1 ↗(On Diff #171628)

Please add new testcase in a NFC patch before this, and only show different due to this opt here. Thanks.

jedilyn marked 2 inline comments as done.Dec 13 2018, 6:36 PM
jedilyn added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4710 ↗(On Diff #171628)

OK, I'll update it. The following called SelectCC has some specific handling on seteq case.

llvm/test/CodeGen/PowerPC/ppc64-P9-setb.ll
1 ↗(On Diff #171628)

Got it, I'll make a NFC patch.

jedilyn updated this revision to Diff 178417.Dec 16 2018, 5:45 PM

Addressed Jinsong's comments.

jsji added a comment.Dec 17 2018, 2:19 PM

LGTM, thanks for exploiting setb!

jsji accepted this revision.Dec 17 2018, 2:20 PM
This revision is now accepted and ready to land.Dec 17 2018, 2:20 PM
jsji retitled this revision from [Power9] Exploit power9 new instruction setb to [PowerPC] Exploit power9 new instruction setb.Dec 17 2018, 3:03 PM
This revision was automatically updated to reflect the committed changes.