Baseline tests for: https://reviews.llvm.org/D133919
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Add negative tests where one of the select arms is a zero? Also, at least 1 test where the predicate is not equality, and a couple of tests where only one pair of the select operands are matching.
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
34 | None of these commuted variants are logically different than the 1st test; they really just swap variable names. I don't think they're adding much value. |
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
34 | Removed the commuted variants. |
This set of tests LGTM, but they are still not enough to provide coverage for the proposed code patch, so we need to add a few more tests. If the pattern-matching uses ValueTracking calls (isKnownNonZero and haveNoCommonBitsSet) then we should have some tests with non-constant select operands to exercise that. You can look at the implementations of those calls and reverse engineer tests that will match with variable operands.
One for each of the non-constant cases should be enough. We're just trying to show that we can (and intended to) handle that kind of pattern via isKnownNonZero or whatever other ValueTracking calls that are used in the transform.
@spatel here we go! Everything should be tested now. Let me know if I am missing something.
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
152 | This (and similar other tests) doesn't provide the coverage that you were hoping for. |
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
152 | Okay, shall I remove them? |
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
152 | Yes - we don't want tests for other transforms as part of this patch. |
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
152 | The latest patchset should solve this ... |
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll | ||
---|---|---|
43 | There are still a bunch of lines like this that reduce immediately to a constant. Those lines should be removed and replace the uses of that value with the constant in the rest of the test. If the test doesn't provide any new coverage with that adjustment, then remove the whole test. | |
87 | clear all but low bit? | |
166 | This is a no-op. Was this supposed to be an 'or %z, 1'? |
Some of the negative tests still evaluate to 'false' or 'true'. I currently have no idea how to fix this ...
We need at least one positive test that exercises the known-bits logic. I was expecting the test that I posted (or something similar) to be in this mix.
Also, something's not working for the ne pred cases - those are miscompiles without a not at the end?
Let's reduce the complexity to make progress:
- If a test is already reduced to true/false, remove it. It doesn't add coverage for the new code.
- Defer the ne part to a follow-up patch. Just add a TODO comment about it in D133919 (also update the patch title so it's not so specific). We can leave the tests here or defer those too; whatever is easier to do is fine.
None of these commuted variants are logically different than the 1st test; they really just swap variable names. I don't think they're adding much value.