This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Baseline tests for folding x + (x | -x) to x & (x - 1)
ClosedPublic

Authored by marcauberer on Sep 7 2022, 1:12 PM.

Diff Detail

Event Timeline

marcauberer created this revision.Sep 7 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 1:12 PM
marcauberer requested review of this revision.Sep 7 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 1:12 PM
spatel added inline comments.Sep 7 2022, 2:26 PM
llvm/test/Transforms/InstCombine/add_or_sub.ll
11

This isn't the pattern you intended - notice the 'or' operands were commuted. For this test only, we don't want any extra 'thwart' instruction.

32–33

This is a fun example. In order, for x to be op0 of the 'add', it must be a binop or other instruction with high complexity. But for x to be op1 of the 'or', it must be equal or lower complexity than a negate - which is treated as a special-case (lower complexity than a binop).

Therefore, the intended pattern is impossible given the current weights. Let's just leave this test as-is in case the weightings ever change.

spatel added inline comments.Sep 7 2022, 2:29 PM
llvm/test/Transforms/InstCombine/add_or_sub.ll
89

Replace undef with poison. We're moving away from undef as a value in IR, so we don't want to add new tests with undef.

Remove extra 'thwart' instruction for the first test

Replace undef with poison in tests

marcauberer marked 3 inline comments as done.Sep 7 2022, 2:38 PM
spatel added inline comments.Sep 7 2022, 3:43 PM
llvm/test/Transforms/InstCombine/add_or_sub.ll
93

This looks good, but there should be 3 of these multi-use tests:

  1. Extra use of the sub only.
  2. Extra use of the or only.
  3. Extra use of both sub and or (that's what we have currently).

The general rule for instcombine transforms is that we don't want to end up with more instructions than we started with, so it can get a bit tricky, but we'll sort that out in the code patch.

Add additional multiuse test cases

marcauberer marked an inline comment as done.Sep 7 2022, 3:58 PM
marcauberer added inline comments.
llvm/test/Transforms/InstCombine/add_or_sub.ll
93

Done 👍

spatel accepted this revision.Sep 7 2022, 4:04 PM

LGTM - the tests are often harder to get right than the code. :)
If you don't have commit access, please post name+email to receive the proper author citation.

This revision is now accepted and ready to land.Sep 7 2022, 4:04 PM
marcauberer marked an inline comment as done.Sep 7 2022, 4:08 PM

Thanks!

I do not have commit access, so here is my data:

Name: Marc Auberer
Username: marcauberer
Email: contact@marc-auberer.com

This revision was landed with ongoing or failed builds.Sep 7 2022, 4:13 PM
This revision was automatically updated to reflect the committed changes.