This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Lower bitreverse in ISel
ClosedPublic

Authored by Rin on May 13 2021, 5:59 AM.

Details

Summary

Adding lowering support for bitreverse.

Previously, lowering bitreverse would expand it into a series of other instructions. This patch makes it so this produces a single rbit instruction instead.

Diff Detail

Unit TestsFailed

Event Timeline

Rin created this revision.May 13 2021, 5:59 AM
Rin requested review of this revision.May 13 2021, 5:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 13 2021, 5:59 AM
Rin edited the summary of this revision. (Show Details)May 13 2021, 6:03 AM
Rin added a reviewer: dmgreen.

Sounds good.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
4134

We can remove this. And Can we remove the definition on int_aarch64_neon_rbit now too?

llvm/test/CodeGen/AArch64/bitreverse.ll
72–73

Can we add tests for all these types:
v8i8 v16i8
v4i16 v8i16
v2i32 v4i32
v1i64 v2i64
Then use update_llc_test_checks to generate the check lines. Also i32 and i64 would be good to have in here.

Bonus points for pre-committing with the old codegen, to just show changes here.

Rin updated this revision to Diff 345411.May 14 2021, 5:36 AM

Remove unnecessary comment and add more bitreverse tests

Rin marked 2 inline comments as done.May 14 2021, 5:37 AM
dmgreen added inline comments.May 14 2021, 7:10 AM
llvm/lib/IR/AutoUpgrade.cpp
556

Do we have a test for the autoupgrade, to show that @llvm.aarch64.neon.rbit turns into @llvm.bitreverse?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
381

Can we move this to the other "hasNeon" section?

1019

Perhaps here. This looks quite neon.

Rin updated this revision to Diff 345465.May 14 2021, 9:37 AM

Add AutoUpgrade test and move bitreverse lowering

Rin marked 3 inline comments as done.May 14 2021, 9:38 AM
dmgreen accepted this revision.May 17 2021, 3:40 AM

Thanks. LGTM

llvm/test/CodeGen/AArch64/neon_rbit.ll
9 ↗(On Diff #345465)

v8i8 and v8i16 are fine for this patch. They others were not valid aarch64.neon.rbit intrinsics anyway.

Also, it may be better to make this an opt test, I'm not sure. It's probably fine either way.

This revision is now accepted and ready to land.May 17 2021, 3:40 AM
This revision was automatically updated to reflect the committed changes.