Page MenuHomePhabricator

[PowerPC] Implement Set Boolean Condition Instructions
AcceptedPublic

Authored by amyk on Sep 15 2020, 9:03 AM.

Details

Reviewers
power-llvm-team
nemanjai
lei
NeHuang
Group Reviewers
Restricted Project
Summary

This patch implements the set boolean condition instructions introduced in POWER10.

The set boolean condition instructions (set[n]bc[r]) are used during these situations:

  • sign/zero/any extending i1 to an i32 or i64,
  • reg+reg, reg+imm or floating point comparisons being sign/zero extended to i32 or i64,
  • spilling CR bits (using the setnbc instruction

Depends on D86252.

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

amyk created this revision.Sep 15 2020, 9:03 AM
amyk requested review of this revision.Sep 15 2020, 9:03 AM
amyk updated this revision to Diff 291966.Sep 15 2020, 10:28 AM

Update the run line of crbits.ll test.

NeHuang added inline comments.
llvm/test/CodeGen/PowerPC/p10-spill-creq.ll
5

Use --check-prefix=CHECK-P10 here?

llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
5

Use --check-prefix=CHECK-P10 here?

llvm/test/CodeGen/PowerPC/p10-spill-crlt.ll
5

Use --check-prefix=CHECK-P10 here to be consistent with other IR cases

llvm/test/CodeGen/PowerPC/p10-spill-crun.ll
5

Use --check-prefix=CHECK-P10 here to be consistent with other IR cases?

llvm/test/CodeGen/PowerPC/testComparesi32gtu.ll
82

Are we missing the instructions check between setbcr3, gt and blr?

llvm/test/CodeGen/PowerPC/testComparesi32ltu.ll
82

Are we missing the instructions check between setbcr3, lt and blr?

lei added a subscriber: lei.Thu, Oct 22, 6:16 AM

For the p10 specific tests, I think just use the default CHECK vs CHECK-P10.
Also, please ensure we are testing on both BE and LE.

llvm/test/CodeGen/PowerPC/CompareEliminationSpillIssue.ll
17

nit: indentation

llvm/test/CodeGen/PowerPC/crbits.ll
4

nit: indentation

12

Do we not need a run line to test LE?

llvm/test/CodeGen/PowerPC/p10-setbc-ri.ll
5

BE test?
Since this is specifically written for P10, maybe just use the default CHECK instead of CHECK-P10.

llvm/test/CodeGen/PowerPC/p10-setbc-rr.ll
5

same.

llvm/test/CodeGen/PowerPC/testComparesi32leu.ll
15

I am not sure what this line is verifying. The --implicit-check-not identicates it's making sure that we are not generating p8/p9 set boolean instructions since it's checking for cmpw and not cmplw which is generated for P10. However, the run line does not contain the option to turn on integer compare codegen ...

llvm/test/CodeGen/PowerPC/testComparesi32ltu.ll
10

Do we want to add and tests for integer compare codegen on P10?

amyk added inline comments.Thu, Oct 22, 4:45 PM
llvm/test/CodeGen/PowerPC/p10-spill-creq.ll
5

Since it's a P10 file, I will omit the CHECK-P10 and just use the default CHECK instead in the other files, as per the suggestion.

llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
5

Since it's a P10 file, I will omit the CHECK-P10 and just use the default CHECK instead in the other files, as per the suggestion.

llvm/test/CodeGen/PowerPC/p10-spill-crlt.ll
5

Since it's a P10 file, I will omit the CHECK-P10 and just use the default CHECK instead in the other files, as per the suggestion.

llvm/test/CodeGen/PowerPC/p10-spill-crun.ll
5

Since it's a P10 file, I will omit the CHECK-P10 and just use the default CHECK instead in the other files, as per the suggestion.

llvm/test/CodeGen/PowerPC/testComparesi32gtu.ll
82

Actually, the code is a bit different on LE/BE.
On LE, the next line is actually b fn2@notoc.
On BE, there is other code, followed by blr. It will probably be good to show this.

llvm/test/CodeGen/PowerPC/testComparesi32leu.ll
15

Yes, you're right. Those shouldn't be needed. I left them there by accident - they'll be removed in the updated diff.

llvm/test/CodeGen/PowerPC/testComparesi32ltu.ll
10

That is a good question and may not be a bad idea. I think I can add them and if there are any objections to their addition, I can remove them.

82

Will be updating the checks.

amyk updated this revision to Diff 300130.Thu, Oct 22, 6:18 PM

Address the following review comments:

  • indentation
  • updated RUN and CHECK lines
lei accepted this revision.Fri, Oct 23, 6:19 AM

LGTM. Thanks for the update.
Please address the additional comments on commit.

llvm/test/CodeGen/PowerPC/p10-setbc-ri.ll
438

no such label...

llvm/test/CodeGen/PowerPC/testComparesi32gtu.ll
25

I think the implicit checks here need to be diff for pwr10. From what I can see... instead of cmpw, on p10 it generates cmplw. So shouldn't we be using that as the implicity check value?

llvm/test/CodeGen/PowerPC/testComparesi32leu.ll
21

same comment for implicit checks.

This revision is now accepted and ready to land.Fri, Oct 23, 6:19 AM
NeHuang accepted this revision.Fri, Oct 23, 9:58 AM

LGTM. please address Lei's comment on commit.