This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] recognizeBSwapOrBitReverseIdiom - recognise zext(bswap(trunc(x))) patterns (PR39793)
ClosedPublic

Authored by RKSimon on Sep 25 2020, 9:15 AM.

Details

Summary

PR39793 demonstrated an issue where we fail to recognize 'partial' bswap patterns of the lower bytes of an integer source.

In fact, most of this is already in place collectBitParts suitably tags zero bits, so we just need to correctly handle this case by finding the zero'd upper bits and reducing the bswap pattern just to the active demanded bits.

Diff Detail

Event Timeline

RKSimon created this revision.Sep 25 2020, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 9:15 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon requested review of this revision.Sep 25 2020, 9:15 AM
RKSimon added inline comments.
llvm/test/Transforms/InstCombine/bswap.ll
293

This looks like a minor separate issue

RKSimon added inline comments.
llvm/test/Transforms/InstCombine/bswap.ll
293
lebedev.ri added inline comments.Sep 29 2020, 10:44 AM
llvm/lib/Transforms/Utils/Local.cpp
3027–3029

Why is this restricted to powers of two?

RKSimon planned changes to this revision.Sep 29 2020, 11:06 AM
RKSimon added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
3027–3029

Good catch - my solution was too focused on the original test cases - I'm looking at refactoring this now.

lebedev.ri added inline comments.Sep 29 2020, 11:13 AM
llvm/lib/Transforms/Utils/Local.cpp
3027–3029

I think this might suggest that there's second half of the puzzle missing,
i.e. should this also know how to widen/pad the inputs so that it is applicable for bswap?

RKSimon marked an inline comment as not done.Sep 29 2020, 11:30 AM
RKSimon added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
3027–3029

yes - shifted bswaps would be possible as a future development - a lot of the existing bitTransformIsCorrectForBSwap code would need to be refactored for that though.

RKSimon updated this revision to Diff 295067.Sep 29 2020, 11:31 AM

I've rewritten the leading zero count code to support any type width and added a 'PR39793_bswap_u50_as_u16' test case

lebedev.ri accepted this revision.Sep 29 2020, 11:44 AM

Looks good to me. @spatel ?

llvm/lib/Transforms/Utils/Local.cpp
3026–3037

Suggestion:

while(!BitProvenance.empty() && BitProvenance.back() < 0)
  BitProvenance = BitProvenance.drop_back();
if(BitProvenance.empty())
  ???
DemandedTy = IntegerType::get(I->getContext(), BitProvenance.size());
This revision is now accepted and ready to land.Sep 29 2020, 11:44 AM
spatel added inline comments.Sep 29 2020, 12:53 PM
llvm/lib/Transforms/Utils/Local.cpp
3027

Should this check and the later one be for == BitPart::Unset rather than <0?
Can we assert that BitProvenance is not <0 other than Unset?

lebedev.ri added inline comments.Sep 30 2020, 4:15 AM
llvm/lib/Transforms/Utils/Local.cpp
3040

Super minor: this then should be if (BitProvenance.back() == BitPart::Unset) {