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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

In D141412#4041351 <https://reviews.llvm.org/D141412#4041351>, @goldstein.w.n wrote:

> In D141412#4041223 <https://reviews.llvm.org/D141412#4041223>, @nikic wrote:
>
>> 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.



================
Comment at: llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll:332
+  %ygt = icmp ugt i32 %y, %x
+  call void @llvm.assume(i1 %ygt)
+
----------------
goldstein.w.n wrote:
> nikic wrote:
> > 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?
> > 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
> ```
> 
In that case, shouldn't the assume below this be dropped? Otherwise that's a contradiction.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll:354
+  %ctpop = call i32 @llvm.ctpop.i32(i32 %y)
+  %yp2 = icmp ne i32 %ctpop, 1
+  call void @llvm.assume(i1 %yp2)
----------------
goldstein.w.n wrote:
> nikic wrote:
> > Doing something like `ule 1` would be more meaningful, in that it is "nearly" pow2.
> > 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`
Good point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141412/new/

https://reviews.llvm.org/D141412