- Add checks if X and/or Y are odd. The Odd values are unnecessary to the icmp: isZero(Odd * N) == isZero(N)
- If neither X nor Y is known odd, then if X * Y cannot overflow AND if X and/or Y is non-zero, the non-zero values are unnecessary to the icmp.
goldstein.w.n on Jan 2 2023, 11:21 AM.Authored by
Both odd: https://alive2.llvm.org/ce/z/9qgwMo
From the test diffs, I don't see any cases where we actually hit the true/false case. Presumably, this get's reliably handled by the "icmp eq isKnownNonZero, 0" fold in InstSimplify. If that's the case, we can omit handling for that.
Thanks for writing those.
For future reference, I ran alive2 on the entire test file locally (basically opt -passes=instcombine tests-before.ll -o tests-after.ll; ./alive2 tests-before.ll tests-after.ll Is there a way to get that into godbolt or is the only way to do 1 function at a time with src and tgt?
We start hitting it in: https://reviews.llvm.org/D140851
Before then the missing cases in analysis was getting in the way. Could probably create a case where
I think it only works with src/tgt. It is also possible to let alive-tv run passes over the input, but of course that requires the changes to already be implemented.
It's not really clear to me that that folds only in conjunction with this patch. I suspect that one also folds via InstSimplify, just with strengthened assume handling.
Thanks, is D140849 also good to push (as a general question, if there is a tests patch + a real patch, does real patch approval imply test patch approval).
Are the tests okay? Or do they need work?
Generally speaking, tests can be committed without review. That said...
I'm still rather unhappy about the tests for this patch. There are both way too many tests, and at the same time not enough of the tests we care about (e.g. there doesn't seem to be a single test using ne, and negative test coverage is pretty much impossible to make out in a 5000 line test file).
Here is what I would expect to be tested in some form:
Having all of these as orthogonal tests is fine, but some can also be combined. For example, you can have the X odd test use a scalar, and the Y odd test use a vector. You can have one establish oddness via or 1, and the other via an assume. Have some tests use an eq predicate, and some use an ne predicate. What there definitely shouldn't be is a combinatorial explosion of all combinations. We don't need a vector variant of each test, and we don't need to test many different ways of specifying known bits / non-zero for each pattern.
So there should be a total of 8 - 16 (approximately) tests here (depending on how orthogonal the tests are, and how thorough you want to be), each covering some important variation of the pattern. This makes it easy to both see that all relevant tests are present, including negative tests.
It may be worthwhile to take a look at some commits by @spatel / rotateright in the git log of llvm/test/Transforms/InstCombine to get an idea of how InstCombine test coverage usually looks like.
I hope this is helpful. I've tried to be overly detailed here, because the same basic problem also exists in other patches, e.g. D142270. I kind of regret bringing up vector test coverage, because having vector variants of every single test wasn't my intention (let alone for vector assumes, which aren't supported at all).
The one addition is IMO select/br for establishing zero/nonzero.
Okay, removed 1/3 from each test from in the D142270 patches and ~4k lines from this one.
I'll be more reserved in the future. My general default is if in doubt test it, but guess thats not
Thank you for the help :)
Either this or D141875 cause the failure: https://lab.llvm.org/buildbot/#/builders/183/builds/10447
Going to revert while I look into it.
Bit confused by the failure, https://lab.llvm.org/buildbot/#/builders/183/builds/10448 which included this commit (and D141875) succeeded.