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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

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.

> 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?



================
Comment at: llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll:332
+  %ygt = icmp ugt i32 %y, %x
+  call void @llvm.assume(i1 %ygt)
+
----------------
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
```



================
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)
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141412