[PATCH] D140850: [InstCombine] Add optimizations for icmp eq/ne (mul(X, Y), 0)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D140850#4083749 <https://reviews.llvm.org/D140850#4083749>, @goldstein.w.n wrote:

> In D140850#4083702 <https://reviews.llvm.org/D140850#4083702>, @nikic wrote:
>
>> Implementation LGTM
>
> 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).

Generally speaking, tests can be committed without review. That said...

> Are the tests okay? Or do they need work?

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:

- X odd
- Y odd
- X non-zero nuw
- Y non-zero nsw
- (Possibly also Y non-zero nuw, X non-zero nsw for completeness)
- (Possibly also X and Y odd / X and Y non-zero, which will already fold before the patch)
- X non-zero, no nowrap flags (negative test)
- icmp constant not zero (negative test)
- icmp predicate not equality (negative test)
- eq / ne predicates [maybe combined with other tests]
- multi-use test (pass mul to `call @use()`) [maybe combined with other tests]
- vector test [maybe combined with other tests]
- assume to establish odd / non-zero [maybe combined with other tests]

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 <https://reviews.llvm.org/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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140850