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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133362