[PATCH] Reassociate in favor of grouping previously paired operands

[Daniel Berlin]

On Mon, Jun 8, 2015 at 5:49 PM, Xuetian Weng <xweng at google.com> wrote:
> On Mon, Jun 8, 2015 at 4:54 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> On Mon, Jun 8, 2015 at 4:23 PM, Xuetian Weng <xweng at google.com> wrote:
>> > Related discussion and original patch on llvmdev.
>> >
>> > http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-May/085246.html
>> >
>> > There are some differences between the original patch and this version:
>> >
>> > 1. Because OptimizeExpression has some assumptions about the order of linearized ops, in this version the order of operands is adjusted after calling function OptimizeExpression.
>> > 2. Because RewriteExprTree can only pair the last two operands in Ops, in this patch the order of operands is not adjusted by rank but by simply moving one pair of operands to the end of Ops.
>>
>> This seems a bit more dangerous?
>> In effect, you are changing the rank.  Tying this to rewritexprtree's
>> current implementation seems like asking for bugs in the fuure
>
> Actually it's less dangerous after tried several way to rank the operands on
> the existing test case. In this patch, operands will be ranked with
> their original
> rank first. OptimizeExpression would do whatever old optimization it want like
> constant combine and factorization. If we try to give them new rank directly
> you would surprisingly find all other original optimizations in
> Reassociate are broken.

This sounds like something that should be fixed, but ...
>
> There's not much thing to be done between OptmizeExpression and
> RewriteExprTree, so it's safe to reorder the optimized operands again there.
>
> As for tying this to rewriteexprtree's implementation, the rank system itself
> is tied to rewritexprtree's implementation.

???
The rank system is 100% independent of how the trees are rewritten.
So i'm not sure what you mean by this.

>
>> > 3. Paired operands are indexed with their corresponding Opcode and Instruction. So (add a, b) will not make a, b together in a multiplication expression.
>>
>> What is the downside to trying to keep operands together that appear
>> together in some other operation?
>>
>> By doing this, you are effectively giving an operand different ranks
>> in different operations.
>>
> For example,
> e  =  a | b
> foo(e)
> ...
> f = b + c
> foo(f)
> ...
> x = a + b
> y = x + c
> foo(y)
>
> Pairing a,b together for y is pointless and would lose opportunity to
> pair b,c together.
>
Okay.

>> > 7. Last pairing map is cleared after processing every basic block.
>>
>> Why?
>> This just seems wrong?
> To avoid dominance issue discussed here. For now I prefer to keep it simple
> and conservative.

Okey, then you bear the burden of demonstrate it is definitively
better than what is there now (it probably is, but actual data
needed!)