Page MenuHomePhabricator

[InstCombine] Change order of canonicalization of ADD and AND
ClosedPublic

Authored by foad on Jul 19 2022, 5:43 AM.

Details

Summary

Canonicalize ((x + C1) & C2) --> ((x & C2) + C1) for suitable constants
C1 and C2, instead of the other way round. This should allow more
constant ADDs to be matched as part of addressing modes for loads and
stores.

Diff Detail

Event Timeline

foad created this revision.Jul 19 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 5:43 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
foad requested review of this revision.Jul 19 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 5:43 AM

This patch subsumes D129844.

I'm not sure what sort of testing is required to show that it improves (or at least doesn't degrade) codegen.

llvm/test/Transforms/InstCombine/or.ll
164

This regression is unfortunate but I don't know what to do about it. Perhaps we can just live with it?

RKSimon added inline comments.Jul 19 2022, 7:27 AM
llvm/test/Transforms/InstCombine/add.ll
727

Update the comment

foad updated this revision to Diff 445836.Jul 19 2022, 9:04 AM

Update test comment

foad marked an inline comment as done.Jul 19 2022, 9:04 AM
spatel added inline comments.Jul 26 2022, 6:30 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1895

C->isNegatedPowerOf2() ?

llvm/test/Transforms/InstCombine/or.ll
158

Please put a TODO note on this test that says this could reduce to t1+2.

foad updated this revision to Diff 447666.Jul 26 2022, 6:42 AM

Address review comments.

foad marked 2 inline comments as done.Jul 26 2022, 6:45 AM
spatel accepted this revision.Jul 26 2022, 7:24 AM

LGTM - see inline for one more test upate.

The question of other logic + math combos came up in the previous review. It looks like we don't handle those either way:
https://github.com/llvm/llvm-project/issues/56731

So this patch should be generalized as a next step (although I have no idea if that would uncover other problems).

llvm/test/Transforms/InstCombine/add.ll
771

This is an existing issue, but this test doesn't add any value as-is. We should be testing if %and has an extra use, not %x.

This revision is now accepted and ready to land.Jul 26 2022, 7:24 AM
foad updated this revision to Diff 447682.Jul 26 2022, 7:35 AM

Fix multi use test

foad marked an inline comment as done.Jul 26 2022, 7:35 AM
foad updated this revision to Diff 454516.Aug 22 2022, 8:42 AM

Rebase.

This revision was landed with ongoing or failed builds.Aug 22 2022, 12:04 PM
This revision was automatically updated to reflect the committed changes.