[PATCH] D41574: [Transforms] Adding a WeakReassociate pass

[Omer Paparo Bivas via Phabricator via llvm-commits]

opaparo added a comment.

In https://reviews.llvm.org/D41574#964524, @opaparo wrote:

> In https://reviews.llvm.org/D41574#964124, @davide wrote:
>
> > In https://reviews.llvm.org/D41574#964111, @opaparo wrote:
> >
> > > In https://reviews.llvm.org/D41574#964073, @davide wrote:
> > >
> > > > It's not entirely clear to me why you need another reassociate pass.
> > > >  We should really try to share as much code as possible with the existing one instead of adding maintenance burden.
> > >
> > >
> > > Although both of the passes perform reassociation, they are essentially very different.
> > >  The Reassociate pass aggressively modifies the topography of the expression tree, basically rewriting it, while the WeakReassociate pass uses the existing topography.
> > >  The Reassociate pass creates new instructions while the WeakReassociate pass does not.
> > >  Moreover, their method of reassociation is completely different. To modify the existing Reassociate pass to supports reassociation of the form (a_0 + a_1) + (a_2 + a_3) -> (a_0 + a_2) + (a_1 + a_3) will take a great amount of changes, changes that will almost certainly collide with existing behavior and test cases. For example, how will it decide whether (a_0 + a_1) + (a_2 + a_3) will be transformed into a_0 + (a_1 + (a_2 + a_3)) like its current behavior or to (a_0 + a_2) + (a_1 + a_3) like the new behavior?
> > >  The existing Reassociate pass lacks, by design, several kinds of reassociations, kinds that I believe can find a home in the new pass.
> > >  IMO, maintaining one big pass that has two distinguished purposes can prove to be a greater burden than maintaining two smaller passes, each dedicated to its own purpose.
> >
> >
> > (Only trying to understand) From what I see the main consumers of reassociation are Global Value Numbering to discover more equivalences and instruction combining.
> >  The former can probably embed reassociation while performing equivalence finding (in fact, there are algorithms where this happens), the latter already performs a fair amount of "canonicalization" (sometimes, too much). I wonder where your pass fits/what's the use case? Do you plan to enable it as part of the default pipeline?
> >
> > Thanks,
> >
> > - Davide
>
>
> The new pass is a kind of preparation pass for InstCombine. It complements changes introduced at https://reviews.llvm.org/D39421, as well as other InstCombine transformations. Checkout my lit tests, they illustrate several scenarios.
>  I believe that this pass will open a gate to other reassociations required by InstCombine transformations, reassociations that can't be expressed in means of the existing Reassociate pass.


@davide, does that answer your questions? Do you have any additional concerns?


Repository:
  rL LLVM

https://reviews.llvm.org/D41574