This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Adjusting bswap pattern to the new masked shl canonization
AbandonedPublic

Authored by opaparo on Dec 18 2017, 7:48 AM.

Details

Summary

A previous patch, D41233, introduced a new canonical form for a masked shl.
As a result, some of the bswap patterns were not recognized, and some bswap tests were temporarily altered.
This patch is a fix for that issue, making sure that those patterns will once again be recognized and transformed.

Diff Detail

Repository
rL LLVM

Event Timeline

hfinkel added inline comments.Dec 19 2017, 12:33 AM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1431–1432

A comment here explaining why (A & B) | (C & D) will be canonicalized into (A << B) | (C & D) in all relevant cases would be useful.

opaparo updated this revision to Diff 127501.Dec 19 2017, 5:07 AM

Added a comment explaining the change of pattern in the bswap idiom detection.

opaparo marked an inline comment as done.Dec 19 2017, 5:07 AM
hfinkel accepted this revision.Dec 19 2017, 7:42 PM

LGTM

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1436

canonic -> canonical

1437

canonic -> canonical

This revision is now accepted and ready to land.Dec 19 2017, 7:42 PM

If I understand the relationship correctly between the patches, this patch would move an existing failure of bswap matching to a different place. Shouldn't we improve the matching instead? Ie, I was expecting an independent patch from D41233 here.

I added tests at rL322206 to demonstrate.

spatel requested changes to this revision.Jan 24 2018, 9:34 AM

Not sure if my question was read - requesting a change based on that.

This revision now requires changes to proceed.Jan 24 2018, 9:34 AM
opaparo abandoned this revision.May 2 2018, 11:47 PM

The required changes were made in D45731