[PATCH] D12345: [Reassociate]: Add intermediate subtract instructions created while negating to be redone later for more reassociate opportunities

[Daniel Berlin via llvm-commits]

First, you can get it to converge in O(size of the largest SCC of the
expressions being evaluated).  Right now, reassociate does not look
through phi nodes, so that size will be 1 :)
Thus, it is possible to converge in one iteration with the right ordering.

Second, rather than spend lots of time on that, i would suggest other
approaches, Jingyu recently suggested a global reassociation algorithm
(https://docs.google.com/document/d/1momWzKFf4D6h8H3YlfgKQ3qeZy5ayvMRh6yR-Xn2hUE/edit#heading=h.pc7256itmioz)

(These are extensions of the existing n-ary reassociate)

This is likely a much better approach than trying to make the local
reassociation pass that reprocess things repeatedly, or even once,
because the *output* will be better :)

In particular,
A. I expect what is there now will not fixpoint in all cases, you'd
have to fix some things.
B.  We already know the heuristics the local reassociation uses are
not only "bad for CSE" in a lot of cases, we know they are optimally
bad in a lot of cases

(IE it will transform things into the least canonical form).

For example:

; foo(a + c);
; foo((a + (b + c));

Reassociate on both:

RAIn: add i32 [ %a, #3] [ %c, #5]
RAOut: add i32 [ %c, #5] [ %a, #3]


and
for the second:
RAIn: add i32 [ %a, #3] [ %b, #4] [ %c, #5]
RAOut: add i32 [ %c, #5] [ %b, #4] [ %a, #3]



(IE c+ a, (c + b) + a)

The longer the expression, the worse it gets.

It is not possible to fix this without a global view of the
expressions, because you need to know "how i i pair the expressions
the last time i saw them" so you can pair them the same way.

Local reassociate, being a local algorithm, only has info about the
current expression chain, and thus, can't do something like this.

TL; DR  While this patch seems great, doing a lot of work on local
reassociate is probably a mistake. Even if you get it to converge in
one iteration (which should be possible given processing), it'll still
give not great results in a lot of cases.

This was also all discussed a few times on the mailing list, if you
look back over threads mentioning reassociate.



On Wed, Aug 26, 2015 at 11:40 AM, Chad Rosier <mcrosier at codeaurora.org> wrote:
> mcrosier added a comment.
>
> In http://reviews.llvm.org/D12345#233382, @aditya_nandakumar wrote:
>
>> I expect when the pass finishes, running reassociate again should not make any changes (the output should already be in canonicalized form - please correct me if this is not a valid expectation).
>
>
> I think this should be the goal, but, as you're finding out, this isn't reality.  IIRC, a similar question was asked and I believe David provided a similar comment.  The approach you're taking seems to be moving us closer and that's a good thing.  We just need to make sure we're not going overboard; we should only revisit things that have changed and only when that change is likely to expose other opportunities.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D12345
>
>
>