This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Recognize another bswap idiom.
ClosedPublic

Authored by chatur01 on Sep 4 2015, 9:09 AM.

Details

Summary

The byte-swap recognizer can now notice that this

uint32_t bswap(uint32_t x)
{
  x = (x & 0x0000FFFF) << 16 | (x & 0xFFFF0000) >> 16;
  x = (x & 0x00FF00FF) << 8 | (x & 0xFF00FF00) >> 8;
  return x;
}

is a bswap. Fixes PR23863.

Diff Detail

Repository
rL LLVM

Event Timeline

chatur01 retitled this revision from to [InstCombine] Recognize another bswap idiom..
chatur01 updated this object.
chatur01 set the repository for this revision to rL LLVM.
chatur01 added a subscriber: llvm-commits.

I'm assuming that the bswap builtin will be triggered in different ways for different target, but that's not what the test here is trying to do, right?

Are there tests to make sure that this builtin expands to the correct instructions in the different types of ARM architectures? If not, I'd add that, too.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2211

You don't seem to have added tests for the AND case.

test/Transforms/InstCombine/bswap.ll
4

Not related to this patch, but we stopped using grep a long time ago...

Maybe it'd be simple enough to add the CHECK lines?

Hi Renato! Thanks for looking at my patch.

I'm assuming that the bswap builtin will be triggered in different ways for different target, but that's not what the test here is trying to do, right?

I thought my transformation was target-independent at this level.

Are there tests to make sure that this built-in expands to the correct instructions in the different types of ARM architectures? If not, I'd add that, too.

There's certainly some, I don't think they're exhaustive of all possibilities. I'll add the test I have to ARM tests at least, but I'll have to do a more intensive sweep of the current tests to make sure they're covering all the possibilities. Sorry for the oversight, this review will have to stall until I've completed that task.

Thanks again Renato.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2211

The test I added checks for the OrOfAnds:
%or6 = or i32 %shl3, %shr5
Where the misnamed "shl3" and "shr5" are and instructions.

test/Transforms/InstCombine/bswap.ll
4

Please can such a change come in a different commit?

rengolin added inline comments.Sep 8 2015, 12:15 PM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2211

Oh, yeah, now I see it. The OR/shifts were already there, too. ok.

test/Transforms/InstCombine/bswap.ll
4

np

Add tests for ARM backend lowering of the BSWAP instrinsic.

The testing coverage in the backend is better now, is this OK to commit?

I'm confused as to why this needs any target specific tests. This is a change to IR canonicalization.

I'm confused as to why this needs any target specific tests. This is a change to IR canonicalization.

I was confused about that too, but I was attempting to address this review comment:

Are there tests to make sure that this built-in expands to the correct instructions in the different types of ARM architectures? If not, I'd add that, too.

Hopefully I won't be asked to go through the other backends as well now! :)

In my experience

I'm confused as to why this needs any target specific tests. This is a change to IR canonicalization.

I was confused about that too, but I was attempting to address this review comment:

Are there tests to make sure that this built-in expands to the correct instructions in the different types of ARM architectures? If not, I'd add that, too.

Hopefully I won't be asked to go through the other backends as well now! :)

Backends are expected to do intelligent things with llvm.bswap, I don't think the burden is on you to make sure they aren't doing the wrong thing.

I didn't mean you should create the tests, I was just asking if there were tests elsewhere about the intrinsic to instructions conversion.

Not that it'd hurt to have the extra tests...

OK, sorry for the misunderstanding. Can I commit my original revision, and land the extra backend tests as part of a separate review?

rengolin accepted this revision.Sep 24 2015, 1:25 AM
rengolin added a reviewer: rengolin.

Yeah, LGTM. Sorry for holding you up.

This revision is now accepted and ready to land.Sep 24 2015, 1:25 AM
chatur01 edited edge metadata.

Roll back to previous revision. I'll add the extra backend tests
in a separate revision.

chatur01 closed this revision.Sep 24 2015, 3:26 AM