http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57297

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
So far I have not attempted to reproduce this myself and so do not
quite follow all the previous comments but...

(In reply to Richard Biener from comment #2)
> 
> build_ref_for_offset ends up creating a MEM_REF with different alias pointer
> type than "the original" (note that there may be multiple originals and
> AFAIK we don't make any attempt to "merge" them conservatively).

...it looks like the second arguments in 
    generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
                 gsi, false, false, loc);
and
    generate_subtree_copies (lacc->first_child, lacc->base, 0, 0, 0,
                 gsi, true, true, loc);

should be the rhs and lhs respectively, rather than [rl]acc->base.
Using rhs/lhs, the generated statements would have the same alias
pointer type as the respective sides of the original statement.  Can
you try that?  We should probably also change accordingly (almost all)
other calls to generate_subtree_copies (this code generally predates
generating MEM_REFs in build_ref_for_offset and this did not matter).

> 
> Re-materializing the original variable will always be hard.  I believe that
> 
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 198420)
> +++ gcc/tree-sra.c      (working copy)
> @@ -3158,7 +3158,7 @@ sra_modify_assign (gimple *stmt, gimple_
>  
>    if (modify_this_stmt
>        || gimple_has_volatile_ops (*stmt)
> -      || contains_vce_or_bfcref_p (rhs)
> +      || contains_bitfld_comp_ref_p (rhs)
>        || contains_vce_or_bfcref_p (lhs))
>      {
>        if (access_has_children_p (racc))
> 
> should work.

Even though relaxing this condition might be a good idea to try to
generate better code, I'd be against fixing bugs this way, if we can
avoid it.  This should be the safe path capable of handling everything
that the latter more sophisticated approaches might choke on.  It
would make the already complex code more difficult to maintain and
sooner or later we'd hit the same problem again (I think it should be
possible use structures with a single field to create a testcase with
a similar problem and modify_this_stmt set to true, for example).

Reply via email to