Page MenuHomePhabricator

[InstCombine] Relax and reorganize one use checks in the ~(a | b) & c
ClosedPublic

Authored by rampitec on Nov 3 2021, 1:22 PM.

Details

Summary

Since there is just a single check for LHS in ~(A | B) & C | ...
transforms and multiple RHS checks inside with more coming I am
removing m_OneUse checks for LHS and adding new checks for RHS.
This is non essential as long as there is total benefit.

In addition (~(A | B) & C) | (~(A | C) & B) --> (B ^ C) & ~A
checks were overly restrictive, it should be good without any
additional checks.

Diff Detail

Event Timeline

rampitec created this revision.Nov 3 2021, 1:22 PM
rampitec requested review of this revision.Nov 3 2021, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 1:22 PM
spatel added inline comments.Nov 9 2021, 1:05 PM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2661–2662

This isn't strong enough to prevent the transform from ending up with more instructions than we started with - we are creating 3 instructions, so we need to eliminate at least 2 intermediate values plus the final value.

I think we'd be ok if we have 2 m_OneUse checks in this match (similar to the transform above this).

Please add a test like this to confirm:

define i32 @or_and_not_not_2_extra_uses(i32 %a, i32 %b, i32 %c) {
  %or1 = or i32 %b, %a
  call void @use(i32 %or1)
  %not1 = xor i32 %or1, -1
  %or2 = or i32 %a, %c
  %not2 = xor i32 %or2, -1
  %and = and i32 %not2, %b
  call void @use(i32 %and)
  %or3 = or i32 %not1, %and
  ret i32 %or3
}
rampitec updated this revision to Diff 385956.Nov 9 2021, 1:31 PM
rampitec marked an inline comment as done.

Added one more m_OneUse.

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2661–2662

Thanks for the catch!

rampitec updated this revision to Diff 385990.Nov 9 2021, 2:59 PM

Fixed second pattern with m_OneUse.

rampitec updated this revision to Diff 385998.Nov 9 2021, 3:50 PM

Rebased to precommited tests.

spatel accepted this revision.Nov 10 2021, 8:37 AM

LGTM

llvm/test/Transforms/InstCombine/and-xor-or.ll
1715

Might want to put an explanatory comment on this test and/or add another test that shows that we have a use-limit in place, but it could be adjusted in the future to be more flexible.

This revision is now accepted and ready to land.Nov 10 2021, 8:37 AM
rampitec updated this revision to Diff 386217.Nov 10 2021, 10:02 AM

Added test comment and rebased.

This revision was landed with ongoing or failed builds.Nov 10 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.