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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D41574