This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N > 0
ClosedPublic

Authored by hkmatsumoto on Mar 30 2022, 12:10 PM.

Details

Summary

(ctpop(X) == N) || (X != 0) --> (X != 0) https://alive2.llvm.org/ce/z/hYL5C2 (for N = 10)
(ctpop(X) != N) && (X == 0) --> (X == 0) https://alive2.llvm.org/ce/z/oLkMvI (for N = 15)

Diff Detail

Event Timeline

hkmatsumoto created this revision.Mar 30 2022, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 12:10 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
hkmatsumoto requested review of this revision.Mar 30 2022, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 12:10 PM
hkmatsumoto added a comment.EditedMar 30 2022, 12:16 PM

Proof (This is first time writing proof in English, so sorry for any inconveniences)
Suppose X: i32

  1. For 0 < N <= 32
    • ctpop(X) == N --> X > 0.
    • Hence (ctpop(X) == N) || (X != 0) --> X > 0 || X != 0.
    • Intuitively X > 0 || X != 0 --> X != 0.
  2. For 32 < N (This case is already optimized anyway)
    • ctpop(X) == N is always false, because of the semantics of ctpop().
    • Hence (ctpop(X) == N) || (X != 0) --> false || X != 0 --> X != 0.

If we are not creating a new value in a transformation (only returning an existing value or constant), we usually prefer to add that fold (and tests) to "InstSimplify".

Note that you can use "llvm.assume" with Alive2 to create a more general proof instead of using a specific constant:
https://alive2.llvm.org/ce/z/vcY7_s

hkmatsumoto updated this revision to Diff 419469.EditedMar 31 2022, 8:13 AM
  • Move change and its test to InstSimplify
  • Remove/unchange tests whose and/or are replaced with select

This is because InstSimlify/and-or-icmp-* tests do not include such tests, and as far as I tried on goldbot.org simplifyAndOrOfICmps* functions in InstructionSimplify.cpp does not acknowledge select operands to optimize.

hkmatsumoto retitled this revision from [InstCombine] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N > 0 to [InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N > 0.Mar 31 2022, 8:16 AM
llvm/lib/Analysis/InstructionSimplify.cpp
1811

Normally in this kind of code, we name this "C" (for constant).

1814

Use m_ZeroInt() instead of m_SpecificInt()?

1815

It seems more natural if we write this as "!C->isZero()"

llvm/test/Transforms/InstCombine/ispow2.ll
900 ↗(On Diff #419469)

We should add a new test for "wrong predicate" because this test and the later one are reduced before we reach the transform in InstCombine.

llvm/test/Transforms/InstSimplify/and-or-icmp-ctpop.ll
35

Should this be or <2 x i1> %cmp, %notzero?

spatel added a comment.Apr 1 2022, 9:44 AM

I posted inline comments on this patch yesterday, but it did not appear to trigger an email.

hkmatsumoto planned changes to this revision.EditedApr 1 2022, 9:55 AM

Oops, sorry I actually got the email but was hidden under a bunch of others.
Let me quickly address some of your reviews, and address all of them tomorrow morning.

Address code reviews

hkmatsumoto marked 4 inline comments as done.Apr 1 2022, 10:07 AM

Oops, sorry I actually got the email but was hidden under a bunch of others.
Let me quickly address some of your reviews, and address all of them tomorrow morning.

Ah - I actually see the mail too now, but it was also buried in my inbox. :)

Thanks for the updates. I think you can request commit access now if you have not already:
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

If you get that, you can add the new tests as an "NFC" patch first. After that, this patch should be good to commit too.

Update negative tests

hkmatsumoto marked an inline comment as done.Apr 2 2022, 9:46 AM

I pre-committed tests but immediately reverted as I noticed I forgot to rerun update_test_checks.py to make them baseline tests.
Let me take some time to confirm that the revert commit (rGf65c78a0949023bb0f3051cdaea7758e48420978) made CI pass.

Sync with main branch

spatel accepted this revision.Apr 4 2022, 4:45 AM

LGTM

This revision is now accepted and ready to land.Apr 4 2022, 4:45 AM
hkmatsumoto retitled this revision from [InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N > 0 to [InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N != 0.Apr 4 2022, 7:21 AM
hkmatsumoto retitled this revision from [InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N != 0 to [InstSimplify] Fold (ctpop(X) == N) || (X != 0) into X != 0 where N > 0.
This revision was landed with ongoing or failed builds.Apr 4 2022, 7:25 AM
This revision was automatically updated to reflect the committed changes.