This is an archive of the discontinued LLVM Phabricator instance.

[NFC][X86][AArch64] Reorganize/cleanup BZHI test patterns
ClosedPublic

Authored by lebedev.ri on May 28 2018, 8:11 AM.

Details

Summary

In D47428, i propose to choose the ~(-(1 << nbits)) as the canonical form of low-bit-mask formation.
As it is seen from these tests, there is a reason for that.

AArch64 currently better handles ~(-(1 << nbits)), but not the more traditional (1 << nbits) - 1 (sic!).
The other way around for X86.
It would be much better to canonicalize.

It would seem that there is too much tests, but this is most of all the auto-generated possible variants
of C code that one would expect for BZHI to be formed, and then manually cleaned up a bit.
So this should be pretty representable, which somewhat good coverage...

Related links:
https://bugs.llvm.org/show_bug.cgi?id=36419
https://bugs.llvm.org/show_bug.cgi?id=37603
https://bugs.llvm.org/show_bug.cgi?id=37610
https://rise4fun.com/Alive/idM

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.May 28 2018, 8:11 AM
lebedev.ri retitled this revision from [NFC][X86][AArch64] Add some more IR tests for BZHI instruction to [NFC][X86][AArch64] Reorganize/cleanup BZHI test patterns.

Ok, yes, those tests were kinda horrible.
I have re-started from scratch, refactored existing tests out of bmi.ll,
re-organized them, structured, and filled missing pieces.
And duplicated to aarch64.

craig.topper added inline comments.May 28 2018, 10:53 AM
test/CodeGen/AArch64/bmi-bzhi.ll
1 ↗(On Diff #148829)

bmi-bzhi doesn't make sense as a name for AArch64. Those are the names of X86 extensions. Someone familiar with AArch64, but not X86 would have no understanding of that.

lebedev.ri added inline comments.May 28 2018, 11:13 AM
test/CodeGen/AArch64/bmi-bzhi.ll
1 ↗(On Diff #148829)

Yes, indeed, i expected that feedback.
Should both of them be named something like extract-low-bits.ll ?

RKSimon added inline comments.May 28 2018, 1:19 PM
test/CodeGen/AArch64/bmi-bzhi.ll
1 ↗(On Diff #148829)

I'm not sure if we gain much from keeping the same filenames on x86 and aarch64 (include a comment in the files pointing to the other target if you think it useful) - on x86 keeping the bmi files together is tidier, I've no opinion on what the aarch64 file should be called.

test/CodeGen/X86/bmi-bzhi.ll
4 ↗(On Diff #148829)

Should we add i686 test coverage for bmi?

test/CodeGen/X86/bmi.ll
3 ↗(On Diff #148829)

Should we add i686 test coverage for bmi?

lebedev.ri added inline comments.May 28 2018, 1:38 PM
test/CodeGen/X86/bmi.ll
3 ↗(On Diff #148829)

Just duplicate every run line with 32-bit triple?
I'll defer to @craig.topper, since i don't know whether that would be useful.

  • Rename bmi-bzhi.ll -> extract-lowbits.ll for both the X86 and AArch64.
  • For now i dropped the tests with truncation after bzhi. I would still like to have that test coverage, but maybe later.

FIXME: so should the 32-bit runlines be added? Which ones specifically?

lebedev.ri marked 2 inline comments as done.May 29 2018, 10:52 AM
lebedev.ri updated this revision to Diff 149627.Jun 3 2018, 2:49 AM
  • Added 32-bit test coverage, following @RKSimon's example. This is completely impossible in the bmi.ll, since llc just crashes on it for 32-bit.
  • Added TBM coverage.

The bmi.ll file is a mess of intrinsics and general combine/pattern tests (which is understandable given so few of the bmi ops are actually inplemented as intrinsics in the headers).

Please can you limit your changes to bmi.ll to pulling out the bzhi tests and I'll work on cleaning up the rest of the file like I did for bmi2

test/CodeGen/X86/extract-lowbits.ll
12 ↗(On Diff #149627)

Given bzhi is a bmi2 op I see very little benefit from testing it on bmi1/tbm only targets, what does it look like if you limit this to 'plain' X86/X64 and BMI2 X86/X64 test?

Also, you don't appear to have left in all the generated code from the update script.

lebedev.ri added inline comments.Jun 3 2018, 6:32 AM
test/CodeGen/X86/bmi.ll
3 ↗(On Diff #148829)

This is a very good idea, in retrospect.
Right now the llc completely crashes for any i686 runline here (e.g. can not select @llvm.x86.bmi.bextr.32),
so i did not add the 32-bit coverage.

test/CodeGen/X86/extract-lowbits.ll
12 ↗(On Diff #149627)

what does it look like if you limit this to 'plain' X86/X64 and BMI2 X86/X64 test?

No.
The bmi1/tbm test coverage is needed.
The next step is to express this pattern using bextr %val (%numlowbits << 8),
when there is no bmi2.
(caveat: i did not yet check that it is profitable as per mca, but i'm pretty sure it should be.)

Also, you don't appear to have left in all the generated code from the update script.

These check prefixes are quite a mystery to me.
I did not manually delete anything here.

lebedev.ri updated this revision to Diff 149633.Jun 3 2018, 6:41 AM

The bmi.ll file is a mess of intrinsics and general combine/pattern tests (which is understandable given so few of the bmi ops are actually inplemented as intrinsics in the headers).

Please can you limit your changes to bmi.ll to pulling out the bzhi tests and I'll work on cleaning up the rest of the file like I did for bmi2

Done.

Thanks - almost there I think. You'll need to rebase to handle the diffs from rL333841

test/CodeGen/X86/extract-lowbits.ll
2 ↗(On Diff #149633)

--check-prefixes=CHECK,X86,NOBMI,X86-NOBMI

7 ↗(On Diff #149633)

--check-prefixes=CHECK,X64,NOBMI,X64-NOBMI

lebedev.ri updated this revision to Diff 149639.Jun 3 2018, 7:36 AM
lebedev.ri marked 2 inline comments as done.

Rebased.

test/CodeGen/X86/extract-lowbits.ll
2 ↗(On Diff #149633)

D'oh, thanks!

RKSimon accepted this revision.Jun 3 2018, 7:40 AM

LGTM - thanks

This revision is now accepted and ready to land.Jun 3 2018, 7:40 AM

LGTM - thanks

Thank you for the review.

This is a precursor for D47453, and may require small further test changes,
i generally try to land stuff like this together with the dependent revision,
so unless this blocks any further cleanup on your side, i'd prefer to wait for D47453?

This revision was automatically updated to reflect the committed changes.