Re: [PATCH]: Fix PR tree-optimization/27373

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

On 5/16/06, Daniel Berlin <dberlin@dberlin.org> wrote:
> This fixes tree-opt/27373, an ICE in add_virtual_operand.
> The actual cause of this bug is that forwprop is not correctly noticing
> it needs to update the SMT info.
>
> I expected forward_propagate_addr_expr_1 to return true if it changed
> something, as that was what i thought it meant when it says "return true
> if propagation is successful".
>
> This is not the case, and it if it only replaces the *LHS*, which is
> what happens here.
>
> Because it didn't return true, we then think nothing has changed, and
> don't touch the SMT info.   In fact, something *did* change.  It
> propagates into the LHS of the expression, and changed the SMT info as a
> result.
>
> However, if we change  the one case it returned false before (around
> line 686):
>
> /* Now see if the LHS node is an INDIRECT_REF using NAME.  If so,
>      propagate the ADDR_EXPR into the use of NAME and fold the result.  */
>   if (TREE_CODE (lhs) == INDIRECT_REF && TREE_OPERAND (lhs, 0) == name)
>     {
>       /* This should always succeed in creating gimple, so there is
>          no need to save enough state to undo this propagation.  */
>       TREE_OPERAND (lhs, 0) = unshare_expr (TREE_OPERAND (stmt, 1));
>       fold_stmt_inplace (use_stmt);
>       tidy_after_forward_propagate_addr (use_stmt);
>     }
>
> so that it returns true, we then end up destroying definitions we
> shouldn't (it looks like it thinks it can remove the statement, which
> must be true of the other cases where we return true).

Yeah, the return value is true only if it did propagate into _all_ uses, so
concerning the stmt propagated to, the def is dead if true is returned.  There
are some corner cases where we can propagate into only the rhs or lhs but
still have uses in the other one left - this is what you are hitting.

> Rather than mess around more with the semantics it currently expects,
> and end up rewriting half the pass, I just added a parameter to
> forward_propagate_addr_expr_1 that says whether anything in use_stmt has
> been changed, which is what we want to know for SMT usage update purposes.

The patch looks good, though I cannot approve it, obviously.

Richard.