Page MenuHomePhabricator

[InstCombine] Combine binary operator of two phi node
AcceptedPublic

Authored by StephenFan on Fri, Mar 3, 12:57 AM.

Details

Reviewers
nikic
spatel
Summary

Combine binary operator of two phi node if there is at least one
specific constant value in phi0 and phi1's incoming values for each
same incoming block and this specific constant value can be used
to do optimization for specific binary operator.
For example:

%phi0 = phi i32 [0, %bb0], [%i, %bb1]
%phi1 = phi i32 [%j, %bb0], [0, %bb1]
%add = add i32 %phi0, %phi1
==>
%add = phi i32 [%j, %bb0], [%i, %bb1]

Fixes: https://github.com/llvm/llvm-project/issues/61137

Diff Detail

Event Timeline

StephenFan created this revision.Fri, Mar 3, 12:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 3, 12:57 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
StephenFan requested review of this revision.Fri, Mar 3, 12:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 3, 12:57 AM

This optimization can be more general, why only add?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1324

call InstSimplify instead of matching this specific pattern?

nikic added a comment.Fri, Mar 3, 1:36 AM

This optimization can be more general, why only add?

Right. This can be generalized at least to ConstantExpr::getBinOpIdentity(), but more generally to performing a simplification on all operands -- the latter might be expensive in terms of compile-time though.

This optimization can be more general, why only add?

Yes, it can be more general. But I want to solve the issue first and left a TODO. Or if you strongly recommend implementing other binary operators in this patch, I am willing to do it.

This optimization can be more general, why only add?

Right. This can be generalized at least to ConstantExpr::getBinOpIdentity(), but more generally to performing a simplification on all operands -- the latter might be expensive in terms of compile-time though.

Okay, let me try to generalize to ConstantExpr::getBinOpIdentity().

Support BinOp Identity.

This comment was removed by hiraditya.
hiraditya added inline comments.Fri, Mar 17, 7:53 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1297

D

1314

nit: any remaining operators we are still missing?

nikic added inline comments.Fri, Mar 17, 8:36 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1314

Might make more sense to mention generalization to inst simplify.

1318

Drop llvm::

1320

This indent is ugly, I'd suggest moving this function into a variable.

1328

No need to dyn_cast, just comparison is enough.

llvm/test/Transforms/InstCombine/phi.ll
1667

Add a test with sub to show that non-commutative operator is not folded?

Move function into a variable and add sub test.

nikic accepted this revision.Sun, Mar 19, 8:03 AM

LG

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1305

values -> value

1306

value -> values :)

1321

Drop braces

llvm/test/Transforms/InstCombine/phi.ll
1644

cannt -> cant or cannot

This revision is now accepted and ready to land.Sun, Mar 19, 8:03 AM