This is an archive of the discontinued LLVM Phabricator instance.

AVX-512: Fixed BT instruction selection on AVX-512
ClosedPublic

Authored by delena on Jul 14 2016, 6:57 AM.

Details

Summary

The following condition expression ( a >> n) & 1 is converted to "bt a, n" instruction. It works on all intel targets.
But on AVX-512 it was broken because the expression is modified to (truncate (a >>n) to i1).

I added the new sequence (truncate (a >>n) to i1) to the BT pattern.

One more thing that required changes in TargetLowering.cpp is bad simplification of (truncate (a >>n) to i1) when we compare it to i1:

%tmp29 = lshr i32 %x, %n               
%tmp3 = and i32 1, %tmp29
%tmp4 = icmp ne i32 %tmp3, 1

is transferred to

 %tmp29 = lshr i32 %x, %n               
%tmp3 = trunc i32 %tmp29 to i1
%tmp4 = icmp eq i32 %tmp3, 0

and then the 'bt' instruction should be selected:

%tmp4 = bt %x, %n

Diff Detail

Repository
rL LLVM

Event Timeline

delena updated this revision to Diff 63963.Jul 14 2016, 6:57 AM
delena retitled this revision from to AVX-512: Fixed BT instruction selection on AVX-512.
delena updated this object.
delena added reviewers: spatel, RKSimon.
delena set the repository for this revision to rL LLVM.
delena added a subscriber: llvm-commits.
spatel added inline comments.Jul 14 2016, 10:27 AM
../lib/Target/X86/X86ISelLowering.cpp
15072–15091 ↗(On Diff #63963)

This is all copied code & comments; please add a helper function instead of duplicating. That should make it easier to read too.

../test/CodeGen/X86/bt.ll
2 ↗(On Diff #63963)

Specify the attribute(s) that are being testing, not a CPU model.
I updated this file at rL275439, so it would be easier to see the current codegen. But I don't know which flavor(s) of avx-512 are affected, so I did not add a RUN for it. It would be better to add that RUN ahead of this patch, so we can see exactly what tests are affected by your patch. Thanks!

delena marked 2 inline comments as done.Jul 17 2016, 2:04 AM

Sanjay, thanks a lot for the review. I'm uploading a new patch.

../lib/Target/X86/X86ISelLowering.cpp
15072–15091 ↗(On Diff #63963)

I'll upload the updated patch.

delena updated this revision to Diff 64244.Jul 17 2016, 2:10 AM
delena marked an inline comment as done.

The changes are made according to Sanjay's comments.

  1. Common code is hoisted to a separate function getBitTestCondition().
  2. The test case bt.ll shows what's done.
spatel accepted this revision.Jul 18 2016, 10:59 AM
spatel edited edge metadata.

LGTM.

../test/CodeGen/X86/bt.ll
28–33 ↗(On Diff #64244)

I removed all of these CHECK lines because I didn't think they added value for any test in this file (we know the correct 'bt' is generated already). If you agree, you could delete all of those checks again.

This revision is now accepted and ready to land.Jul 18 2016, 10:59 AM
This revision was automatically updated to reflect the committed changes.