[PATCH] D140849: [Patch 1/4]: Add tests for binops with conditions/assume constraints

Wed Jan 4 16:34:21 PST 2023

goldstein.w.n added a comment.

In D140849#4026570 <https://reviews.llvm.org/D140849#4026570>, @nikic wrote:

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

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:

1. Add them in this review
2. Add them to the reviews for patches 3/4 respectively
3. Create 2 new reviews (one for each).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140849