[PATCH] D133362: [InstCombine] Fold x + (x | -x) to x & (x - 1)

[Marc Auberer via Phabricator via llvm-commits]

marcauberer marked an inline comment as done.
marcauberer added a comment.

@spatel @RKSimon I now added some more extensive tests, which should cover all relevant cases.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1425-1428
+  // (add A (or A, -A)) --> (and (add A, -1) A)
+  // (add A (or -A, A)) --> (and (add A, -1) A)
+  // (add (or A, -A) A) --> (and (add A, -1) A)
+  // (add (or -A, A) A) --> (and (add A, -1) A)
----------------
spatel wrote:
> This shows the expected set of commuted variants of the general pattern, but I don't think the match is handling all of them.
> 
> You'll want to add tests for each of these variations, and that's going to require some bonus instructions to make the patterns stick until we reach this point in visitAdd().
> 
> Here are some tests that I recently added that can serve as templates - note especially the lines with "thwart complexity-based canoinicalization":
> d4a4004c0f9d3070
As a matter of fact, it seems like the match is covering all four cases. The first four test cases in the test ref file should cover those. I didn't even need the "thwart complexity based canonicalization instructions" 


================
Comment at: llvm/test/Transforms/InstCombine/add_or_sub.ll:26
+  ret i8 %add
+}
----------------
spatel wrote:
> marcauberer wrote:
> > RKSimon wrote:
> > > vector cases? multiuse cases? negative cases?
> > I just added vector and multi-use cases. How do I do negative cases?
> For each positive condition in the match statement(s), create a test that does not satisfy the requirement.
> 
> So for example, we're looking for a negate (sub) of a specific operand in the 'or', then create a test with a negate of a different value:
>   %sub = sub i8 0, %y  <-- mismatch, so this should not transform
>   %or = or i8 %sub, %x
>   %add = add i8 %or, %x
> 
> Or maybe the 'sub' isn't a negate; it might be: (1 - x). Change the 'or' to an 'xor', etc.
> 
Thanks for the explanation! I've added a few negative tests. Please let me know if something is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133362