This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] reassociate bitwise logic chains based on uses
ClosedPublic

Authored by spatel on Aug 7 2022, 8:09 AM.

Details

Summary

(X op Y) op Z --> (Y op Z) op X

This isn't a complete solution (see TODO tests for possible refinements), but it shows some nice wins and doesn't seem to cause any harm. I think the most potential danger is from conflicting with other folds and causing an infinite loop - that's the reason for avoiding patterns with constant operands.

Alternatively, we could try this in the reassociate pass, but we would not immediately see all of the logic folds that instcombine provides. I also looked at improving ValueTracking's isImpliedCondition() (and we should still add some enhancements there), but that would not work in general for bitwise logic reduction.

The tests that reduce completely to 0/-1 are motivated by issue #56653.

Diff Detail

Event Timeline

spatel created this revision.Aug 7 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 8:09 AM
spatel requested review of this revision.Aug 7 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 8:09 AM
spatel edited the summary of this revision. (Show Details)Aug 7 2022, 8:13 AM

Presumably, these are fuzzer bugs, but some more issues that should be solved with this patch:
https://github.com/llvm/llvm-project/issues/57028
https://github.com/llvm/llvm-project/issues/57120

They are not identical patterns, but have the shared trait of a single value with multiple uses in the logic chain.

Presumably, these are fuzzer bugs, but some more issues that should be solved with this patch:
https://github.com/llvm/llvm-project/issues/57028
https://github.com/llvm/llvm-project/issues/57120

They are not identical patterns, but have the shared trait of a single value with multiple uses in the logic chain.

Maybe we can run ipsccp until no changed(Similar to Instcombine) to fix all these tickets. like:

unsigned Iteration = 0;
bool MadeChanges = false;
while (runIPSCCP(M, DL, GetTLI, getAnalysis)) {
  MadeChanges = true;
}

if (!MadeChanges)
  return PreservedAnalyses::all();

Maybe we can run ipsccp until no changed(Similar to Instcombine) to fix all these tickets. like:

I'm not familiar with ipsccp, and I don't know what impact running that pass to fix-point would have on compile-time, but if that works, do you want to make a patch?

It doesn't look like this patch would have any noticeable impact on compile-time:
https://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=rotateright

Maybe we can run ipsccp until no changed(Similar to Instcombine) to fix all these tickets. like:

I'm not familiar with ipsccp, and I don't know what impact running that pass to fix-point would have on compile-time, but if that works, do you want to make a patch?

IPSCCP/SCCP should iterate until they reach a fixed-point already. If running runIPSCCP(M, DL, GetTLI, getAnalysis) multiple times improves the result, then there's likely a dependency missing in the solver.

bcl5980 added a comment.EditedAug 16 2022, 9:40 PM

Maybe we can run ipsccp until no changed(Similar to Instcombine) to fix all these tickets. like:

I'm not familiar with ipsccp, and I don't know what impact running that pass to fix-point would have on compile-time, but if that works, do you want to make a patch?

It doesn't look like this patch would have any noticeable impact on compile-time:
https://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=rotateright

Sorry for the confused comments. This patch is OK for me. It helps on the cases with non-bool types that ipsccp can't tough. Put them to reassociate pass could avoid the potential dead loop in instcombine, but for now reassociate pass still have the history code don't work for i1 types.
And I'm also trying to fix on ipsccp to eliminate all similar issues with i1 types.

bcl5980 added a comment.EditedAug 16 2022, 11:03 PM

Maybe we can run ipsccp until no changed(Similar to Instcombine) to fix all these tickets. like:

I'm not familiar with ipsccp, and I don't know what impact running that pass to fix-point would have on compile-time, but if that works, do you want to make a patch?

IPSCCP/SCCP should iterate until they reach a fixed-point already. If running runIPSCCP(M, DL, GetTLI, getAnalysis) multiple times improves the result, then there's likely a dependency missing in the solver.

This is a very simple case that run once runIPSCCP can't get fix-point result. https://alive2.llvm.org/ce/z/o6rkTy
I create a issue here https://github.com/llvm/llvm-project/issues/57186
Maybe we can discuss there to make this patch's review more clean.

Sorry for the confused comments. This patch is OK for me. It helps on the cases with non-bool types that ipsccp can't tough. Put them to reassociate pass could avoid the potential dead loop in instcombine, but for now reassociate pass still have the history code don't work for i1 types.
And I'm also trying to fix on ipsccp to eliminate all similar issues with i1 types.

Improving ipsccp seems good independently. I also looked at trying to solve the i1 pattern as a special-case using isImpliedCondition() from ValueTracking, but then I tried this instcombine idea, and it worked on the motivating cases plus others.

If there are no objections, would someone approve the patch formally?

bcl5980 accepted this revision.Aug 18 2022, 9:56 PM

LGTM

This revision is now accepted and ready to land.Aug 18 2022, 9:56 PM
This revision was landed with ongoing or failed builds.Aug 21 2022, 6:44 AM
This revision was automatically updated to reflect the committed changes.