Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
High level comment on these test is that many of them test a combination of folds, rather than their primitive parts. E.g. to test D140851 it would be sufficient to just have an assume followed by an and+icmp to check the known bits. Going through the mul fold to test computeKnownBits() behavior is both unnecessarily indirect, and limited: We can only observe the lowest bit through that, so all tests end up doing something like and x, 1 only. Having some end-to-end tests on motivating patterns is fine, but we mainly should be testing things in isolation. (That would probably also cut down on the total number of tests.)
| llvm/test/Transforms/InstCombine/icmp-binop.ll | ||
|---|---|---|
| 16 | All these zext + ret and be replaced by ret i1. | |
| 296 | What's the point of these these tests that contain only assume calls? | |
| 616 | What's the point of these tests? | |
Make {X|Y}Extra clearer. Remove some unnecesisarily verbose code.
Didn't update, was update 2 and 3.
| llvm/test/Transforms/InstCombine/icmp-binop.ll | ||
|---|---|---|
| 16 |
Figured no difference. Do you want all unnecessary zext removed? | |
| 296 | These tests are more for patterns 3/4. | |
| 616 |
None, I write the tests in C -> ll. Guess this one got optimized. I'll drop in next revision to this patch. | |
Fair enough, when I wrote this initially I was trying to fix a missed optimization in the mul case so wrote my tests based on that.
We can only observe the lowest bit through that, so all tests end up doing something like and x, 1 only. Having some end-to-end tests on motivating patterns is fine, but we mainly should be testing things in isolation. (That would probably also cut down on the total number of tests.)
Is your preference for me to remove some of these tests? My general feeling is more is better than less, even if they become
a bit redundant. When working on the patch (only working with u32) I think at some point or another just about every case
broke at some point so I generally see them as useful.
The s64 case (that was able to optimize before the patch b.c of NSW) is useless so will drop, but the other "trivially"
different cases seem worth keeping.
Regarding new tests for patches 3/4. Would your preference be:
- Add them in this review
- Add them to the reviews for patches 3/4 respectively
- Create 2 new reviews (one for each).
I think we're stilling missing a test where the icmp constant is not zero.
| llvm/test/Transforms/InstCombine/icmp-binop.ll | ||
|---|---|---|
| 26 | Either this should be ne or the test name adjusted. | |
| 103 | Was this supposed to be a 0 (or an eq)? Otherwise this is assuming even, not odd. | |
| 247 | I think we're still missing a test where the second op is non-zero, so maybe change %v to %v2 here? | |
All these zext + ret and be replaced by ret i1.