Re: [PATCH] Fix middle-end/38747 (Wrong code due to VIEW_CONVERT_EXPR) and middle-end/38748 (Missed FRE because of VIEW_CONVERT_EXPR)

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

On Fri, Jan 16, 2009 at 11:34 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Jan 16, 2009 at 2:47 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> Hi,
>>  The problem here is that forward prop can cause an aliasing problem
>> that did not exist in the original source.  So currently we transform
>> *(*int)&a->float_field into VIEW_CONVERT_EXPR<int>(a->float_field).
>> This causes an aliasing issue if the aliasing sets of int and float
>> don't overlap.  So we want to disable this transformation if the inner
>> most reference is an INDIRECT_REF and the aliasing sets don't match or
>> the aliasing set for the access of the INDIRECT_REF will be 0.  Just
>> disabling it for all INDIRECT_REFs will disabling some optimizations
>> when the aliasing sets are the same and the VIEW_CONVERT_EXPR could be
>> changed into a NOP_EXPR (think unsigned int and signed int, I added a
>> testcase for that).
>>
>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> I would prefer a more "regular" approach (at least for 4.4) like
>
> Index: tree-ssa-forwprop.c
> ===================================================================
> --- tree-ssa-forwprop.c (revision 143429)
> +++ tree-ssa-forwprop.c (working copy)
> @@ -777,7 +777,7 @@ forward_propagate_addr_expr_1 (tree name
>       && operand_equal_p (TYPE_SIZE (TREE_TYPE (rhs)),
>                          TYPE_SIZE (TREE_TYPE (TREE_OPERAND (def_rhs,
> 0))), 0))
>    {
> -     tree new_rhs = unshare_expr (TREE_OPERAND (def_rhs, 0));
> +     tree def_rhs_base, new_rhs = unshare_expr (TREE_OPERAND (def_rhs, 0));
>      new_rhs = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (rhs), new_rhs);
>      if (TREE_CODE (new_rhs) != VIEW_CONVERT_EXPR)
>        {
> @@ -788,16 +788,23 @@ forward_propagate_addr_expr_1 (tree name
>         new_rhs = force_gimple_operand_gsi (use_stmt_gsi, new_rhs, true, NULL,
>                                             true, GSI_NEW_STMT);
>         gimple_assign_set_rhs1 (use_stmt, new_rhs);
> +        tidy_after_forward_propagate_addr (use_stmt);
> +        return true;
>        }
> -     else
> +     /* If the defining rhs comes from an indirect reference, then do not
> +        convert into a VIEW_CONVERT_EXPR.  */
> +     def_rhs_base = TREE_OPERAND (def_rhs, 0);
> +     while (handled_component_p (def_rhs_base))
> +       def_rhs_base = TREE_OPERAND (def_rhs_base, 0);
> +     if (!INDIRECT_REF_P (def_rhs_base))
>        {
>         /* We may have arbitrary VIEW_CONVERT_EXPRs in a nested component
>            reference.  Place it there and fold the thing.  */
>         *rhsp = new_rhs;
>         fold_stmt_inplace (use_stmt);
> +        tidy_after_forward_propagate_addr (use_stmt);
> +        return true;
>        }
> -     tidy_after_forward_propagate_addr (use_stmt);
> -     return true;
>    }
>
>   /* If the use of the ADDR_EXPR is not a POINTER_PLUS_EXPR, there
>
> did you mean to name the testcases str_i_ct-aliasing*?
>
> Thanks,
> Richard.

I will go ahead, take my patch and your testcases. put this
through a bootstrap / test cycle and apply it.

Richard.

>> Thanks,
>> Andrew Pinski
>>
>> * tree-ssa-forwprop.c (forward_propagate_addr_expr_1): Disable the VCE
>> conversion if the base address is an indirect reference and the
>> aliasing sets could cause issues.
>>
>> * gcc.dg/tree-ssa/struct-aliasing-1.c: New test.
>> * gcc.dg/tree-ssa/struct-aliasing-2.c: New test.
>> * gcc.c-torture/execute/struct-aliasing-1.c: New test.
>>
>