Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[InstCombine] Fold or(phi1,phi2) into or(icmp1,icmp2)
AbandonedPublic

Authored by bipmis on Sep 11 2023, 10:03 AM.

Details

Summary

For a specific pattern of type or(phi1, phi2), where the other use of phi is an "icmp ne with 0", we can reuse these icmps to form the argument to or. This helps reuse the icmps, reduce or to and, and also bring icmps close to phi for further optimizations.

Alive link https://alive2.llvm.org/ce/z/eAyHWm

Diff Detail

Event Timeline

bipmis created this revision.Sep 11 2023, 10:03 AM
bipmis requested review of this revision.Sep 11 2023, 10:03 AM
RKSimon edited reviewers, added: goldstein.w.n; removed: RKSimon.Sep 13 2023, 3:06 AM
bipmis updated this revision to Diff 556866.Sep 15 2023, 9:25 AM

Update patch with a separate function definition to handle cmp(or(phi,phi)).
New test added without loops.

goldstein.w.n added inline comments.Sep 19 2023, 11:09 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2030

This seems very much so like a DAG type fold where you are basing the decision on existing expressions.
phi doesn't exist in backend (dagcombiner) so its a bit tricky.
@nikic, any place this can be put where it doesn't req iterating the use-list?

generally in instcombine we get our duplication by just blindly canoniclizing to a standard format.

Is there a regression if you blindly do the transform?

bipmis added inline comments.Sep 19 2023, 12:00 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2030

I am seeing quite some benefit with this Transform as we are able to reuse instructions which happens to be the other use of phi.
However just the direct transform
or(i64 a,i64 b) -> or((icmp ne a ,0), (icmp ne b ,0)) is expensive. On the contrary the reverse is true in InstCombine.

I can check if the transform or(i64 phia,i64 phib) regresses.

goldstein.w.n added inline comments.Sep 19 2023, 1:36 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2030

There are a lot of folds that we can do / improve if we optimized around "what exists". If there is a place for such folds (prior to DAGCombiner) that would want this to be moved there. Maybe nikic has an idea about that.

bipmis abandoned this revision.Sep 28 2023, 5:58 AM

Closing this as it may result in reverse fold and can be handle in a better way.