Re: [PATCH Atom][PR middle-end/44382] Tree reassociation improvement

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Jul 19, 2011 at 4:46 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hello Richard,
>
> Thanks a lot for the review!
>
>> it's not easy to follow the flow of this function, esp. I wonder
>>
>> + ? ? ?else
>> + ? ? ? {
>> + ? ? ? ? tree var = create_tmp_reg (TREE_TYPE (last_rhs1), "reassoc");
>> + ? ? ? ? add_referenced_var (var);
>> + ? ? ? ? stmts[i] = build_and_add_sum (var, op1, op2, opcode);
>> + ? ? ? }
>>
>> what you need to create new stmts for, and if you possibly create
>> multiple temporary variables here.
>>
>> - ?TODO_verify_ssa
>> + ?TODO_remove_unused_locals
>> + ? ?| TODO_verify_ssa
>>
>> why?
>>
>
> Current rewrite_expr_tree uses original statements and ?just change
> order of operands (tree leafs) usage. To keep data flow correct it
> moves all statements down to the last one. I tried such approach but
> it did not work well because of increased registers pressure in some
> cases (all operands are live at the same time right before the first
> statement of our sequence). Then I tried to move existing statements
> to the appropriate places (just after the last operand definition) but
> it appeared to be not so easy. It is easy to guarantee consistence in
> the final result but not easy to keep data flow consistent after each
> statement move. And if we do not keep code consistent after each move
> then we may get wrong debug statement generated. After few tries I
> decided that the most easy for both understanding and implementation
> way is to recreate statements and remove the old chain. So, I used
> build_and_add_sum to create all statements except the last one which
> never needs recreation. I added TODO_remove_unused_locals since temps
> used in original statements should be removed. Is this approach OK?

Well, it's enough to delay that to later passes that do this, so I'd prefer
to not change this in this patch.

>> You don't seem to handle the special-cases of rewrite_expr_tree
>> for the leafs of your tree, especially the PHI node special-casing.
>> I think you may run into vectorization issues here.
>>
> I do not see any reason why these cases are checked in
> rewrite_expr_tree. It is optimization of operands order and may be it
> will be good to move it somewhere. Is it OK if I move related code to
> the end of optimize_ops_list method?

I suppose yes.  Maybe the cases are also obsoleted by the improved
loop PHI handling.

Richard.

> Thanks
> Ilya
>