This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine]: Add tests for icmp ne non-zero power of 2; NFC
ClosedPublic

Authored by goldstein.w.n on Jan 10 2023, 10:58 AM.

Details

Reviewers
nikic
spatel

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 10 2023, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 10:58 AM
goldstein.w.n requested review of this revision.Jan 10 2023, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 10:58 AM

Generally looks fine, but I would omit most of the tests on different bitwidths. Sometime we have one test for a different bitwidth (picking something weird like i13) to show that it doesn't matter, but otherwise testing just a single small bitwidth like i8 or i16 is enough (small bitwidth preferred to avoid alive2 timeouts). Not going to insist though.

I'm assuming the non-constant tests are there for the sake of completeness, and there is no intent to support them.

llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll
333

Not sure I understand the significance of this test. It seems like the same as the previous with this extra assume, but why is it relevant?

355

Doing something like ule 1 would be more meaningful, in that it is "nearly" pow2.

Add ule ctpop test case

Generally looks fine, but I would omit most of the tests on different bitwidths. Sometime we have one test for a different bitwidth (picking something weird like i13) to show that it doesn't matter, but otherwise testing just a single small bitwidth like i8 or i16 is enough (small bitwidth preferred to avoid alive2 timeouts). Not going to insist though.

Prefer more tests for the simple cases, the patch to not-break sign-extension by shrinking bitwidth "passing" b.c
tests lacked variety in constants / type has pretty much convinced its worth over testing with slight variation. So,
if its all the same would like to keep. But your the maintainer so LMK.

I'm assuming the non-constant tests are there for the sake of completeness, and there is no intent to support them.

Nothing in the works to support them now, but I think they should be supportable. They aren't going to be covered by
any of the patches I have out now but plan to revisit. Since they cover the same logic okay to keep?

llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll
333

Not sure I understand the significance of this test. It seems like the same as the previous with this extra assume, but why is it relevant?

if (Pow2(Y) && Y > X)
   X & Y -> 0
355

Doing something like ule 1 would be more meaningful, in that it is "nearly" pow2.

Adding as seperate test. See: pow2_or_zero_32_nonconst_assume_br

That changes the evaluation.

if (Pow2OrZero(Y) && (Y & X) != 0)
    X & Y -> Y

whereas just ne 1 can't modify Y & X

nikic accepted this revision.Jan 10 2023, 1:38 PM

Generally looks fine, but I would omit most of the tests on different bitwidths. Sometime we have one test for a different bitwidth (picking something weird like i13) to show that it doesn't matter, but otherwise testing just a single small bitwidth like i8 or i16 is enough (small bitwidth preferred to avoid alive2 timeouts). Not going to insist though.

Prefer more tests for the simple cases, the patch to not-break sign-extension by shrinking bitwidth "passing" b.c
tests lacked variety in constants / type has pretty much convinced its worth over testing with slight variation. So,
if its all the same would like to keep. But your the maintainer so LMK.

Fair enough.

I'm assuming the non-constant tests are there for the sake of completeness, and there is no intent to support them.

Nothing in the works to support them now, but I think they should be supportable. They aren't going to be covered by
any of the patches I have out now but plan to revisit. Since they cover the same logic okay to keep?

Sure, having the tests makes sense. But unless there's an actual motivating case, I don't think supporting those is worthwhile.

llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll
333

In that case, shouldn't the assume below this be dropped? Otherwise that's a contradiction.

355

Good point.

This revision is now accepted and ready to land.Jan 10 2023, 1:38 PM
goldstein.w.n marked an inline comment as done.

Fix inconsistent test

Fix inconsisntent test (forgot to apply last time)

goldstein.w.n marked an inline comment as done.Jan 10 2023, 1:44 PM
goldstein.w.n added inline comments.
llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll
333

In that case, shouldn't the assume below this be dropped? Otherwise that's a contradiction.

You're right, done in V3.

Assume for for ule as well