This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by goldstein.w.n on Jan 23 2023, 5:38 PM.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 23 2023, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 5:38 PM
Herald added a subscriber: foad. · View Herald Transcript
goldstein.w.n requested review of this revision.Jan 23 2023, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 5:38 PM

Remove 1/3 of tests

Cover all new cases in tests

nikic added inline comments.Feb 20 2023, 11:58 AM
llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
16

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?)

goldstein.w.n added inline comments.Feb 20 2023, 12:24 PM
llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
16

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.

goldstein.w.n added inline comments.Feb 20 2023, 12:26 PM
llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
33

Will remove this, sorry had thought I had dropped all the tests that already evaluated.

Simplify tests

goldstein.w.n added inline comments.Feb 20 2023, 1:47 PM
llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
16

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?)

nikic accepted this revision.Feb 22 2023, 2:08 PM

LGTM

This revision is now accepted and ready to land.Feb 22 2023, 2:08 PM
This revision was landed with ongoing or failed builds.Feb 23 2023, 5:52 PM
This revision was automatically updated to reflect the committed changes.