This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Baseline tests for folding and-of-selects to xor
AbandonedPublic

Authored by marcauberer on Sep 16 2022, 12:31 PM.

Details

Reviewers
spatel
RKSimon
Summary

Diff Detail

Event Timeline

marcauberer created this revision.Sep 16 2022, 12:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:31 PM
marcauberer requested review of this revision.Sep 16 2022, 12:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:31 PM

Add negative tests where one of the select arms is a zero? Also, at least 1 test where the predicate is not equality, and a couple of tests where only one pair of the select operands are matching.

llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
34

None of these commuted variants are logically different than the 1st test; they really just swap variable names. I don't think they're adding much value.

Add negative tests

Delete superflous commuted tests and reflect changes from eq to the ne test file

marcauberer marked an inline comment as done.Oct 3 2022, 1:17 PM
marcauberer added inline comments.
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
34

Removed the commuted variants.

marcauberer marked an inline comment as done.

Remove multi-use tests

Fix 0 select arm tests

spatel added a comment.Oct 6 2022, 5:31 AM

This set of tests LGTM, but they are still not enough to provide coverage for the proposed code patch, so we need to add a few more tests. If the pattern-matching uses ValueTracking calls (isKnownNonZero and haveNoCommonBitsSet) then we should have some tests with non-constant select operands to exercise that. You can look at the implementations of those calls and reverse engineer tests that will match with variable operands.

Add test cases for checking non-immediate select arms

@spatel is one test case for icmpeq and one for icmpne enough for checking this?

@spatel is one test case for icmpeq and one for icmpne enough for checking this?

One for each of the non-constant cases should be enough. We're just trying to show that we can (and intended to) handle that kind of pattern via isKnownNonZero or whatever other ValueTracking calls that are used in the transform.

Add more tests for ValueTracking API calls

@spatel here we go! Everything should be tested now. Let me know if I am missing something.

spatel added inline comments.Oct 28 2022, 12:24 PM
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
152

This (and similar other tests) doesn't provide the coverage that you were hoping for.
This will be constant-folded before we ever reach the transform that we're hoping to unit-test.
That's why the test is already completely reduced to "false". :)

marcauberer added inline comments.Oct 28 2022, 12:27 PM
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
152

Okay, shall I remove them?

spatel added inline comments.Oct 28 2022, 12:41 PM
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
152

Yes - we don't want tests for other transforms as part of this patch.
But we still need tests to verify that the transform to be added is behaving as expected.
See if something like this works:
https://alive2.llvm.org/ce/z/2er3z7
...and adjust as needed (make the false arm of the selects non-immediate too), so the tests provide coverage for more of the pattern variations.

Test the right things:

marcauberer marked an inline comment as done.Nov 22 2022, 9:39 AM
marcauberer added inline comments.
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
152

The latest patchset should solve this ...

spatel retitled this revision from [InstCombine] Baseline tests for folding ((x?1:4)&(y?1:4))==0 to x^y to [InstCombine] Baseline tests for folding and-of-selects to xor.Nov 22 2022, 10:18 AM
spatel added inline comments.Nov 22 2022, 10:24 AM
llvm/test/Transforms/InstCombine/select_and_icmpeq.ll
43

There are still a bunch of lines like this that reduce immediately to a constant. Those lines should be removed and replace the uses of that value with the constant in the rest of the test.

If the test doesn't provide any new coverage with that adjustment, then remove the whole test.

87

clear all but low bit?

166

This is a no-op. Was this supposed to be an 'or %z, 1'?

marcauberer marked an inline comment as done.

Apply improvements

marcauberer marked 3 inline comments as done.Nov 23 2022, 12:55 PM

Some of the negative tests still evaluate to 'false' or 'true'. I currently have no idea how to fix this ...

We need at least one positive test that exercises the known-bits logic. I was expecting the test that I posted (or something similar) to be in this mix.

Also, something's not working for the ne pred cases - those are miscompiles without a not at the end?

Let's reduce the complexity to make progress:

  1. If a test is already reduced to true/false, remove it. It doesn't add coverage for the new code.
  2. Defer the ne part to a follow-up patch. Just add a TODO comment about it in D133919 (also update the patch title so it's not so specific). We can leave the tests here or defer those too; whatever is easier to do is fine.
marcauberer abandoned this revision.May 21 2023, 1:37 PM