Patchwork Fix ICE during RTL expansion at -O1

login
register
mail settings
Submitter Richard Guenther
Date April 15, 2013, 9:47 a.m.
Message ID <CAFiYyc3RxZL7joMZYX5K6r3SKDmJxqRacP9z9XmxPto2qSAK+g@mail.gmail.com>
Download mbox | patch
Permalink /patch/236549/
State New
Headers show

Comments

Richard Guenther - April 15, 2013, 9:47 a.m.
On Sun, Apr 14, 2013 at 9:46 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This is a quadratic algorithm and as such not ok.  We already have
>> aliasing_component_refs_p in tree-ssa-alias.c which is supposed to be
>> the non-quadratic replacement.  It's not used via decl_refs_may_alias_p,
>> so that may be the thing to fix.
>
> aliasing_component_refs_p isn't powerful enough, it eliminates the quadratic
> aspect by assuming that all offsets are constants, so it misses cases like
> (*p)[i].f1 vs a[j].f2.  Moreover it assumes TBAA and we don't need it here.

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).

> I can rewrite nonoverlapping_component_refs_of_decl_p to make it non-quadratic
> and catch the same cases I think, patch attached (without the vect testsuite
> adjustments, but they are still needed).
>
>> nonoverlapping_component_refs_of_decl_p on RTL should go - in fact
>> we do call the tree oracle from all its callers so we only ever do redundant
>> work (after your proposed patch even more so).
>
> Not clear if the tree oracle can catch the above case with *p and a, but, yes,
> nonoverlapping_component_refs_p should go in the long term.
>
>
>         * alias.c (nonoverlapping_component_refs_p): Protect again LTO quirk.
>         * tree-ssa-alias.c (nonoverlapping_component_refs_of_decl_p): New.
>         (decl_refs_may_alias_p): Add REF1 and REF2 parameters.
>         Use nonoverlapping_component_refs_of_decl_p to disambiguate component
>         references.
>         (refs_may_alias_p_1): Adjust call to decl_refs_may_alias_p.
>         * tree-streamer.c (record_common_node): Adjust reference in comment.


without actually removing nonoverlapping_component_refs_p it still applies
to both...


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).

+  /* Pop the stacks in parallel and examine the COMPONENT_REFs of the same
+     rank.  This is sufficient because you cannot reference several fields
+     at a time (unlike for arrays with slices), unless you're in a union,
+     in which case the return value will be false in any case.  */
+  while (true)

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?

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.

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?

Thanks,
Richard.

>
> --
> Eric Botcazou
Richard Guenther - April 15, 2013, 12:43 p.m.
On Mon, Apr 15, 2013 at 11:47 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sun, Apr 14, 2013 at 9:46 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> This is a quadratic algorithm and as such not ok.  We already have
>>> aliasing_component_refs_p in tree-ssa-alias.c which is supposed to be
>>> the non-quadratic replacement.  It's not used via decl_refs_may_alias_p,
>>> so that may be the thing to fix.
>>
>> aliasing_component_refs_p isn't powerful enough, it eliminates the quadratic
>> aspect by assuming that all offsets are constants, so it misses cases like
>> (*p)[i].f1 vs a[j].f2.  Moreover it assumes TBAA and we don't need it here.
>
> 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).
>
>> I can rewrite nonoverlapping_component_refs_of_decl_p to make it non-quadratic
>> and catch the same cases I think, patch attached (without the vect testsuite
>> adjustments, but they are still needed).
>>
>>> nonoverlapping_component_refs_of_decl_p on RTL should go - in fact
>>> we do call the tree oracle from all its callers so we only ever do redundant
>>> work (after your proposed patch even more so).
>>
>> Not clear if the tree oracle can catch the above case with *p and a, but, yes,
>> nonoverlapping_component_refs_p should go in the long term.
>>
>>
>>         * alias.c (nonoverlapping_component_refs_p): Protect again LTO quirk.
>>         * tree-ssa-alias.c (nonoverlapping_component_refs_of_decl_p): New.
>>         (decl_refs_may_alias_p): Add REF1 and REF2 parameters.
>>         Use nonoverlapping_component_refs_of_decl_p to disambiguate component
>>         references.
>>         (refs_may_alias_p_1): Adjust call to decl_refs_may_alias_p.
>>         * tree-streamer.c (record_common_node): Adjust reference in comment.
>
> 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.  Instead, as the case passes
>
>               if (typex == typey)
>                 goto found;
>
> earlier you should assert that DECL_CONTEXT (fieldx) == DECL_CONTEXT
> (fieldy) == typex == typey here.  Note that fails of this test are
> expected even in the
> non-LTO case because I cannot find any IL verification that would verify
> that for a COMPONENT_REF TREE_TYPE (TREE_OPERAND (cr, 0))
> == DECL_CONTEXT (TREE_OPERAND (cr, 1)) (due to sharing of the FIELD_DECL
> chain between different type variants the check will fail for all
> non-main-variants
> I think, so refining it to look at the main variant is probably advised).
>
> 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.
>
> 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...
>
>
> 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).
>
> +  /* Pop the stacks in parallel and examine the COMPONENT_REFs of the same
> +     rank.  This is sufficient because you cannot reference several fields
> +     at a time (unlike for arrays with slices), unless you're in a union,
> +     in which case the return value will be false in any case.  */
> +  while (true)
>
> 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?
>
> 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.
>
> 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).

Testcase:

struct S {
    int i;
    int j;
};
struct U {
    struct S s;
} __attribute__((may_alias));
int __attribute__((noinline,noclone))
foo (struct U *p, struct U *q)
{
  int i;
  q->s.j = 1;
  i = p->s.i;
  return i;
}
int main()
{
  int *p = (int *)__builtin_alloca (sizeof (int) * 3);
  p[1] = 0;
  if (foo ((struct U *)(p + 1), (struct U *)p) != 1)
    __builtin_abort ();
  return 0;
}

fails on x86_64 with -O2 -fschedule-insns because scheduling exchanges
the store and the load.

Richard.

>
> Also with your patch enhanced like I suggest we should be able to
> remove aliasing_component_refs_p as well, no?
>
> Thanks,
> Richard.
>
>>
>> --
>> Eric Botcazou

Patch

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.  Instead, as the case passes

              if (typex == typey)
                goto found;

earlier you should assert that DECL_CONTEXT (fieldx) == DECL_CONTEXT
(fieldy) == typex == typey here.  Note that fails of this test are
expected even in the
non-LTO case because I cannot find any IL verification that would verify
that for a COMPONENT_REF TREE_TYPE (TREE_OPERAND (cr, 0))
== DECL_CONTEXT (TREE_OPERAND (cr, 1)) (due to sharing of the FIELD_DECL
chain between different type variants the check will fail for all
non-main-variants
I think, so refining it to look at the main variant is probably advised).

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.

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);
     }
 }