[PATCH] D59758: [DAGCombiner] Combine OR as ADD when no common bits are set

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D59758#1442959 <https://reviews.llvm.org/D59758#1442959>, @spatel wrote:

> In D59758#1441124 <https://reviews.llvm.org/D59758#1441124>, @bjope wrote:
>
> > Hello reviewers! Do you think this is a good idea?
>
>
> It's an interesting idea. :)
>
> > I've mostly seen improvements for our OOT target when doing this, but for example llvm/test/CodeGen/X86/split-store.ll also exposes a case when we trigger a rewrite into using SUB.
>
> Yes, we'd classify that as a slight regression for x86.


Isn't split-store.ll showing an improvement (we get one subb instead of andb+orb)?

However, signbit-shift.ll might show a regression (since we get one more instruction and use an extra register). However, I'm not that familiar with the vector instructions to understand if it really is a regression (maybe those movdqa instructions are easier to schedule, or having shorter latency or something).

> Do you have a sense of how many different folds we're missing in the tests where you show improvements? If it's a small number, we're probably better off just duplicating that code inside 'visitOR', so we don't have to deal with the regressions.

We got lots of selection patterns using "add", for example patterns involving addressing modes, multiply-and-add, etc. Currently our OOT target is using some hooks to rewrite ADD->OR in the first rounds of DAG combiner, and then in the last DAG combiner run we instead rewrite OR->ADD. That is a little bit hacky, so I'm trying to avoid that and either duplicate patterns (detecting or-with-no-common-bits in tablegen patterns), or rewriting to "add" in the PreprocessISelDAG hook. It was when working on that I noticed lots of regressions due to no longer doing the DAG combines that would trigger if using ADD instead of OR in the last DAG combine run.

Examples:

- not triggering the reassociation like ((X + C) + Y)  => ((X + Y) + C) resulted in some regressions
- not triggering the combines involving SUB such as ((0 - A) + B) => (B - A) resulted in some regressions

I can try to make it more selective (duplicating some specific folds). I actually started out that way, but then I figured that it might be better to do all possible folds from visitADD.

Normally we try to do most folds in visitADD before we do rewrite into OR (so that should be the normal case when we have ADD in the input). But if for example opt starts rewriting ADD->OR so we have OR already in the input we won't even try those folds before we rewrite ADD->OR. This makes the DAG combiner sensitive to the input form IMHO. This could be amended by this patch, but if I start to be more selective we most likely will still end up with different results for semantically equivalent input.
Note however that I'm not only focusing on being less sensitive to if ADD or OR has been used in the input to SelectionDAG. Some of the problems I've seen is related to order of combines/lowering, such as doing the ADD->OR rewrite before we have done other simplifications that would have triggered the folds in visitADD if we for example had delayed the ADD->OR rewrite to after legalization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59758