This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Optimise bitreverse lowering in ISel
ClosedPublic

Authored by Rin on May 25 2021, 11:24 AM.

Details

Summary

Some Code Generation optimisation for lowering bit reverse in ISel. It uses vrev32 and vrev64 to optimise the lowering of bit reverse in the cases when VT is v4i32 and v2i64.

Diff Detail

Event Timeline

Rin created this revision.May 25 2021, 11:24 AM
Rin requested review of this revision.May 25 2021, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 11:24 AM
Rin edited the summary of this revision. (Show Details)May 25 2021, 11:27 AM
Rin added reviewers: dmgreen, SjoerdMeijer.
SjoerdMeijer added inline comments.May 25 2021, 11:52 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6868

Perhaps an assert here for VT == MVT::v4i32 || VT == MVT::v2i64.

6869

How about these SVE checks, do we need tests for that?

6872

Nit: /*OverrideNEON=*/true ?

And we don't need the curly brackets for this if.

6879

then this can be an else.

6881

Wanted add a nit about Bitreverse -> BitReverse, but perhaps we don't need it:

return DAG.getNode(AArch64ISD::NVCAST, DL, VT,
                   DAG.getNode(ISD::BITREVERSE, DL, MVT::v16i8, REVB));
Matt added a subscriber: Matt.May 25 2021, 1:31 PM

Can we add v2i32 and maybe v1i64 handling too?

Rin added inline comments.May 26 2021, 3:44 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6869

I think those are already tested for. That's just old code for the SVE case that I put here to make sure I don't change anything for SVE

Rin updated this revision to Diff 347941.May 26 2021, 5:56 AM
Rin marked 3 inline comments as done.
Rin edited the summary of this revision. (Show Details)

Add optimisation for v2i32 and v1i64 bitreverse lowering and respond to review comments

Rin marked an inline comment as done.May 26 2021, 5:57 AM
Rin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6872

Should I get rid of the

/*OverrideNEON=*/true

above in LowerCTTZ as well?

6879

changed it to switch statements

dmgreen added inline comments.Jun 1 2021, 12:42 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6872

I think he meant to add /*OverrideNEON=*/, as it makes the code easier to read.

6873

If we have the unreachable below, we probably don't need the assert as well.

6884

As the code in each of these case blocks are very similar, is it worth having the switch just set some variables (BitReverseType and RevOpcode), and having the actual code shared below the switch?

Rin marked an inline comment as done.Jun 1 2021, 12:53 AM
Rin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6872

Oh, I misunderstood, I'll add it back in

6873

I'll get rid of it

6884

Okay, I'll take care of that then.

Rin updated this revision to Diff 348933.Jun 1 2021, 4:17 AM
Rin marked an inline comment as done.

Address review comments

Rin marked an inline comment as done.Jun 1 2021, 4:18 AM
dmgreen accepted this revision.Jun 2 2021, 12:22 AM

Thanks. LGTM.

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

You can probably remove some of this extra whitespace.

This revision is now accepted and ready to land.Jun 2 2021, 12:22 AM
This revision was landed with ongoing or failed builds.Jun 2 2021, 4:51 AM
This revision was automatically updated to reflect the committed changes.

Since this change LLVM :: Analysis/CostModel/AArch64/bitreverse.ll is failing.

https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/5/logs/FAIL__LLVM__bitreverse_ll

In case it's not just a forgotten test (didn't fail in buildkite which is odd), you can find the config used here https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/3/logs/stdio.

Rin added a comment.Jun 2 2021, 6:11 AM

Since this change LLVM :: Analysis/CostModel/AArch64/bitreverse.ll is failing.

https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/5/logs/FAIL__LLVM__bitreverse_ll

In case it's not just a forgotten test (didn't fail in buildkite which is odd), you can find the config used here https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/3/logs/stdio.

Sorry about that. This new test wasn't caught and I am working on updating it right now to add a fix.

thakis added a subscriber: thakis.Jun 2 2021, 6:51 AM

If it takes a while to fix, can we revert this in the meantime to get the bots back to green?

Rin added a comment.Jun 2 2021, 6:53 AM

If it takes a while to fix, can we revert this in the meantime to get the bots back to green?

it doesn't take much at all, just retesting now to make sure everything is fine so no need to revert.

Rin added a comment.Jun 2 2021, 7:04 AM

Since this change LLVM :: Analysis/CostModel/AArch64/bitreverse.ll is failing.

https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/5/logs/FAIL__LLVM__bitreverse_ll

In case it's not just a forgotten test (didn't fail in buildkite which is odd), you can find the config used here https://lab.llvm.org/buildbot/#/builders/43/builds/6828/steps/3/logs/stdio.

Should be fixed now

Rin added a comment.Jun 2 2021, 7:35 AM

No worries. Sorry about the failures earlier :)