[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 inline comments.


================
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:
> marcauberer wrote:
> > 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" 
> The patch doesn't transform this example:
> 
> ```
> define i32 @add_or_sub_comb_i32_variant1(i32 %p) {
>   %x = mul i32 %p, %p ; thwart complexity-based canonicalization
>   %sub = sub i32 0, %x
>   %or = or i32 %x, %sub
>   %add = add i32 %or, %x
>   ret i32 %add
> }
> ```
> 
> It will be easier to see what's happening if we commit the baseline tests first (and we'll do that anyway), but you should probably create the expected tests for commuted patterns first.
I think I got now what you mean. I added the "thwart complexity-based canonicalization" to every positive test and fixed the bug in the code. Could you take another look?

The baseline tests are now live here: https://reviews.llvm.org/D133449


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133362