Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

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

On Thu, Aug 11, 2016 at 1:09 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
>
> On 10/08/16 20:28, Richard Biener wrote:
>>
>> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>>>
>>>> I see it now. The problem is we are just looking at (-1) being in the
>>>> ops
>>>> list for passing changed to rewrite_expr_tree in the case of
>>>> multiplication
>>>> by negate.  If we have combined (-1), as in the testcase, we will not
>>>> have
>>>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>>>
>>>> We should set changed based on what happens in try_special_add_to_ops.
>>>> Attached patch does this. Bootstrap and regression testing are ongoing.
>>>> Is
>>>> this OK for trunk if there is no regression.
>>>
>>>
>>> I think the bug is elsewhere.  In particular in
>>> undistribute_ops_list/zero_one_operation/decrement_power.
>>> All those look problematic in this regard, they change RHS of statements
>>> to something that holds a different value, while keeping the LHS.
>>> So, generally you should instead just add a new stmt next to the old one,
>>> and adjust data structures (replace the old SSA_NAME in some ->op with
>>> the new one).  decrement_power might be a problem here, dunno if all the
>>> builtins are const in all cases that DSE would kill the old one,
>>> Richard, any preferences for that?  reset flow sensitive info + reset
>>> debug
>>> stmt uses, or something different?  Though, replacing the LHS with a new
>>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of
>>> a
>>> user var that doesn't yet have any debug stmts.
>>
>>
>> I'd say replacing the LHS is the way to go, with calling the appropriate
>> helper
>> on the old stmt to generate a debug stmt for it / its uses (would need
>> to look it
>> up here).
>>
>
> Here is an attempt to fix it. The problem arises when in
> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added
> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
> zero_one_operation. Unlike what was done earlier, we now change the stmt
> (with propagate_op_to_signle use or by directly) such that the value
> computed by stmt is no longer what it used to be. Because of this, what is
> computed in undistribute_ops_list and rewrite_expr_tree are also changed.
>
> undistribute_ops_list already expects this but rewrite_expr_tree will not if
> we dont pass the changed as an argument.
>
> The way I am fixing this now is, in linearize_expr_tree, I set ops_changed
> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call
> zero_one_operation with ops_changed = true, I replace all the LHS in
> zero_one_operation with the new SSA and replace all the uses. I also call
> the rewrite_expr_tree with changed = false in this case.
>
> Does this make sense? Bootstrapped and regression tested for
> x86_64-linux-gnu without any new regressions.

I don't think this solves the issue.  zero_one_operation associates the
chain starting at the first *def and it will change the intermediate values
of _all_ of the stmts visited until the operation to be removed is found.
Note that this is independent of whether try_special_add_to_ops did anything.

Even for the regular undistribution cases we get this wrong.

So we need to back-track in zero_one_operation, replacing each LHS
and in the end the op in the opvector of the main chain.  That's basically
the same as if we'd do a regular re-assoc operation on the sub-chains.
Take their subops, simulate zero_one_operation by
appending the cancelling operation and optimizing the oplist, and then
materializing the associated ops via rewrite_expr_tree.

Richard.

> Thanks,
> Kugan
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * gcc.dg/tree-ssa/pr72835.c: New test.
>
> gcc/ChangeLog:
>
> 2016-08-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR tree-optimization/72835
>         * tree-ssa-reassoc.c (zero_one_operation): Incase of NEGATE_EXPR
> create and use
>          new SSA_NAME.
>         (try_special_add_to_ops): Return true if we changed the value in
> operands.
>         (linearize_expr_tree): Return true if try_special_add_top_ops set
> ops_changed to true.
>         (undistribute_ops_list): Likewise.
>         (reassociate_bb): Pass ops_changed returned by linearlize_expr_tree
> to rewrite_expr_tree.
>
>
>
> whil cif we change the operands such that the
>
> /zero_one_operation