[PATCH] D142426: [ValueTracking] Add tests for KnownBits of (and/xor/or X, (add/sub X, OddV)); NFC

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll:15
+  %b = and i32 %z, 1
+  ret i32 %b
+}
----------------
goldstein.w.n wrote:
> nikic wrote:
> > Ideally, input IR and output IR for test cases are the same (before the patch) to the degree that this is possible. Otherwise it gives the impression that a test covers a codepath it doesn't actually use -- for example, the test is written as if this checks the `sub X, Odd` case, but it really is a convoluted way to write an `add X, Odd` test.
> > 
> > (It looks like a lot of tests here end up testing `add X, 1` in the end -- was that intentional?)
> > Ideally, input IR and output IR for test cases are the same (before the patch) to the degree that this is possible. Otherwise it gives the impression that a test covers a codepath it doesn't actually use -- for example, the test is written as if this checks the `sub X, Odd` case, but it really is a convoluted way to write an `add X, Odd` test.
> 
> Okay, will update to the preexisting generated IR (unless its a transform that loses information and breaks the test).
> 
> 
> > 
> > (It looks like a lot of tests here end up testing `add X, 1` in the end -- was that intentional?)
> 
> Yes, if you do `and X, 1` it commutes and simplifies. `add X, 1` can't commute w.o the known bits so its better for testing the exact case.
> Ideally, input IR and output IR for test cases are the same (before the patch) to the degree that this is possible. Otherwise it gives the impression that a test covers a codepath it doesn't actually use -- for example, the test is written as if this checks the `sub X, Odd` case, but it really is a convoluted way to write an `add X, Odd` test.
> 
> (It looks like a lot of tests here end up testing `add X, 1` in the end -- was that intentional?)

Dramatically simplified tests. Think they are all relatively clear now (maybe too many failure cases?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142426