Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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 |
if (Pow2(Y) && Y > X) X & Y -> 0 | |
355 |
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 |
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. |
llvm/test/Transforms/InstCombine/icmp-ne-pow2.ll | ||
---|---|---|
333 |
You're right, done in V3. |
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?