Re: [patch] Fix ICE during RTL expansion at -O1

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

On Tue, Apr 16, 2013 at 11:55 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Note that looking at the access path _is_ assuming TBAA constraints as
>> soon as the base objects are not the same (in the above case '*p' and 'a'
>> are not the same and p could alias a in a way that all f1 and f2 overlap).
>
> Right, but here I'm assuming (and asserting) that the base objects are the
> same.
>
>> Index: alias.c
>> ===================================================================
>> --- alias.c     (revision 197926)
>> +++ alias.c     (working copy)
>> @@ -2232,8 +2232,11 @@ nonoverlapping_component_refs_p (const_r
>>
>>      found:
>>        /* If we're left with accessing different fields of a structure, then
>> no -        possible overlap, unless they are both bitfields.  */
>> -      if (TREE_CODE (typex) == RECORD_TYPE && fieldx != fieldy)
>> +        possible overlap, unless they are both bitfields.
>> +        ??? Pointer inequality is too fragile in the LTO compiler.  */
>> +      if (TREE_CODE (typex) == RECORD_TYPE
>> +         && fieldx != fieldy
>> +         && DECL_NAME (fieldx) != DECL_NAME (fieldy))
>>
>> this, if at all, should go in with a separate patch and a testcase.
>> And I think it should _not_ go in.
>
> OK, I can remove the LTO related bits.
>
>> Otoh...
>>
>> +      /* ??? We cannot simply use the type of operand #0 of the refs here
>> +        as the Fortran compiler smuggles type punning into COMPONENT_REFs
>> +        for common blocks instead of using unions like everyone else.  */
>> +      tree type1 = TYPE_MAIN_VARIANT (DECL_CONTEXT (field1));
>> +      tree type2 = TYPE_MAIN_VARIANT (DECL_CONTEXT (field2));
>> +
>> +      if (type1 != type2 || TREE_CODE (type1) != RECORD_TYPE)
>> +        goto may_overlap;
>> +
>> +      /* ??? Pointer inequality is too fragile in the LTO compiler.  */
>> +      if (field1 != field2 && DECL_NAME (field1) != DECL_NAME (field2))
>>
>> this suggests you are seeing multiple FIELD_DECLs for the same field
>> in the _same_ FIELD_DECL chain ...?!  Are you sure this happens with
>> GCC 4.8?  There were some fixes in that area in the LTO type merging code.
>
> No, let's drop the LTO related bits for mainline.  But the Fortran related
> bits are necessary, it's the verification you talked about earlier: for a
> COMPONENT_REF
>
>   TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (t, 0)))
>     = TYPE_MAIN_VARIANT (DECL_CONTEXT (TREE_OPERAND (t, 1)))
>
> is expected to be always true (but isn't checked as you pointed out).  But the
> Fortran compiler violates it (4.9, gfortran.dg/aliasing_array_result_1.f90) as
> it implements a common block by defining 2 RECORD_TYPEs with 1 field, one for
> each variables, and does an implicit type conversion of TREE_OPERAND (t, 0).
>
>> Index: tree-streamer.c
>> ===================================================================
>> --- tree-streamer.c     (revision 197926)
>> +++ tree-streamer.c     (working copy)
>> @@ -267,10 +267,9 @@ record_common_node (struct streamer_tree
>>        /* The FIELD_DECLs of structures should be shared, so that every
>>          COMPONENT_REF uses the same tree node when referencing a field.
>>          Pointer equality between FIELD_DECLs is used by the alias
>> -        machinery to compute overlapping memory references (See
>> -        nonoverlapping_component_refs_p).  */
>> -      tree f;
>> -      for (f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
>> +        machinery to compute overlapping component references (see
>> +        nonoverlapping_component_refs_of_decl_p).  */
>> +      for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
>>         record_common_node (cache, f);
>>      }
>>  }
>>
>> without actually removing nonoverlapping_component_refs_p it still applies
>> to both...
>
> Will adjust.
>
>> Can you port the non-quadratic algorithm to the RTL
>> nonoverlapping_component_refs_p first?  The core code should exactly
>> be the same ... (and shared, and only called once from the RTL oracle).
>
> No, it cannot be ported, the non-quadratic aspect is a consequence of the same
> base object assumption.  It you don't have it, you need to be quadratic to be
> able to deal with variable offsets in this way.
>
>> I don't understand the "reference several fields" comment.  Because I
>> can clearly have aggregate copies of RECORD_TYPE.  Can you try
>> do elaborate more on why the algorithm should be sufficient to catch
>> all cases?
>
> I can, but you need to keep in mind that the base objects are the same.
>
>> You could enhance it to not require
>>
>> +  /* We must have the same base DECL.  */
>> +  gcc_assert (ref1 == ref2);
>>
>> for MEM_REF bases under the same conditions like aliasing_component_refs_p,
>> that is if the MEM_REF isn't view-converting.
>
> The code already handles MEM_REF<ADDR_EXPR> though (and checks that the offset
> is zero).  I'm not sure that we need more (but ready to be proved wrong here).
>
>> That said, if the bases are the same DECLs then indeed you do not
>> rely on TBAA.  The RTL nonoverlapping_component_refs_p does not
>> disable itself properly for pointer based accesses that might be
>> view-converted / aliased accesses (a simple testcase with ref-all
>> pointers properly offsetted to alias two adjacent fields may be enough to
>> show that).
>>
>> Also with your patch enhanced like I suggest we should be able to
>> remove aliasing_component_refs_p as well, no?
>
> My revised patch is only a safe, non-quadratic, non-TBAA based disambiguation
> for references based on the same DECL, so it's a full replacement neither for
> aliasing_component_refs_p nor for nonoverlapping_component_refs_p.

Hmm, ok.  Sad :/

> Thanks for the review.  Updated patch attached.

+      if (type1 != type2 || TREE_CODE (type1) != RECORD_TYPE)
+        goto may_overlap;

ick, can TREE_CODE (type1) != RECORD_TYPE happen as well here?
Please add a comment similar to the Fortran ??? above.

Can you please also add at least one testcase as gcc.dg/tree-ssa/ssa-fre-??.c
that tests the functionality of this and that wasn't handled before?
I suppose it
would be sth like

struct S { int i; int j; };
struct U
{
  struct S a[10];
} u;

u.a[n].i= i;
u.a[n].j = j;
return u.a[n].i;

where we miss to CSE the load from u.a[n].i.

Otherwise the patch is ok.

Thanks!
Richard.