[PATCH] Fix PR80222

[PATCH] Fix PR80222

[Richard Biener]

In the context of PR79671 it was noticed that forcing a ref-all
pointer access via a simple static_cast <C&> (obj) and C
with may_alias on it didn't work.

But it should.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2017-03-28  Richard Biener  <rguenther@suse.de>

	PR middle-end/80222
	* gimple-fold.c (gimple_fold_indirect_ref): Do not touch
	TYPE_REF_CAN_ALIAS_ALL references.
	* fold-const.c (fold_indirect_ref_1): Likewise.

	* g++.dg/pr80222.C: New testcase.

Index: gcc/gimple-fold.c
===================================================================
*** gcc/gimple-fold.c	(revision 246500)
--- gcc/gimple-fold.c	(working copy)
*************** gimple_get_virt_method_for_binfo (HOST_W
*** 6539,6546 ****
    return gimple_get_virt_method_for_vtable (token, v, offset, can_refer);
  }
  
! /* Given a pointer value OP0, return a simplified version of an
!    indirection through OP0, or NULL_TREE if no simplification is
     possible.  Note that the resulting type may be different from
     the type pointed to in the sense that it is still compatible
     from the langhooks point of view. */
--- 6539,6546 ----
    return gimple_get_virt_method_for_vtable (token, v, offset, can_refer);
  }
  
! /* Given a pointer value T, return a simplified version of an
!    indirection through T, or NULL_TREE if no simplification is
     possible.  Note that the resulting type may be different from
     the type pointed to in the sense that it is still compatible
     from the langhooks point of view. */
*************** gimple_fold_indirect_ref (tree t)
*** 6554,6560 ****
  
    STRIP_NOPS (sub);
    subtype = TREE_TYPE (sub);
!   if (!POINTER_TYPE_P (subtype))
      return NULL_TREE;
  
    if (TREE_CODE (sub) == ADDR_EXPR)
--- 6554,6561 ----
  
    STRIP_NOPS (sub);
    subtype = TREE_TYPE (sub);
!   if (!POINTER_TYPE_P (subtype)
!       || TYPE_REF_CAN_ALIAS_ALL (ptype))
      return NULL_TREE;
  
    if (TREE_CODE (sub) == ADDR_EXPR)
Index: gcc/fold-const.c
===================================================================
*** gcc/fold-const.c	(revision 246500)
--- gcc/fold-const.c	(working copy)
*************** fold_indirect_ref_1 (location_t loc, tre
*** 14006,14012 ****
  
    STRIP_NOPS (sub);
    subtype = TREE_TYPE (sub);
!   if (!POINTER_TYPE_P (subtype))
      return NULL_TREE;
  
    if (TREE_CODE (sub) == ADDR_EXPR)
--- 14006,14013 ----
  
    STRIP_NOPS (sub);
    subtype = TREE_TYPE (sub);
!   if (!POINTER_TYPE_P (subtype)
!       || TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (op0)))
      return NULL_TREE;
  
    if (TREE_CODE (sub) == ADDR_EXPR)
Index: gcc/testsuite/g++.dg/pr80222.C
===================================================================
*** gcc/testsuite/g++.dg/pr80222.C	(nonexistent)
--- gcc/testsuite/g++.dg/pr80222.C	(working copy)
***************
*** 0 ****
--- 1,13 ----
+ // { dg-do compile }
+ // { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ struct C { int i; }__attribute__((may_alias)) ;
+ 
+ C a, b;
+ 
+ int main()
+ {
+   a = static_cast <C&> (b);
+ }
+ 
+ // { dg-final { scan-tree-dump "{ref-all}\\\)&b\];" "optimized" } } */

Re: [PATCH] Fix PR80222

[Eric Botcazou]

> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
> 
> Richard.
> 
> 2017-03-28  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/80222
> 	* gimple-fold.c (gimple_fold_indirect_ref): Do not touch
> 	TYPE_REF_CAN_ALIAS_ALL references.
> 	* fold-const.c (fold_indirect_ref_1): Likewise.

You just backported it onto the 6 branch and the fold_indirect_ref_1 hunk 
alone introduced regressions in the C testsuite on the branch for strict-
alignment architectures, e.g. SPARC.  Reduced testcase:

typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V b; } P __attribute__((aligned (1)));

__attribute__((noinline, noclone)) void
bar (P *p)
{
  p->b[1] = 5;
}

Before the patch at -O0:

bar:
        save    %sp, -96, %sp
        st      %i0, [%fp+68]
        ld      [%fp+68], %g1
        ldub    [%g1+12], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+12]
        ldub    [%g1+13], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+13]
        ldub    [%g1+14], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+14]
        ldub    [%g1+15], %g2
        and     %g2, 0, %g2
        or      %g2, 5, %g2
        stb     %g2, [%g1+15]
        ldub    [%g1+8], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+8]
        ldub    [%g1+9], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+9]
        ldub    [%g1+10], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+10]
        ldub    [%g1+11], %g2
        and     %g2, 0, %g2
        stb     %g2, [%g1+11]
        nop
        return  %i7+8
         nop


After the patch:

bar:
        save    %sp, -96, %sp
        st      %i0, [%fp+68]
        ld      [%fp+68], %g1
        add     %g1, 8, %g1
        mov     0, %g2
        mov     5, %g3
        std     %g2, [%g1]
        nop
        return  %i7+8
         nop

that is to say, the compiler now generates an unaligned store.

-- 
Eric Botcazou

Re: [PATCH] Fix PR80222

[Richard Biener]

On May 13, 2017 10:49:31 AM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to
>trunk.
>> 
>> Richard.
>> 
>> 2017-03-28  Richard Biener  <rguenther@suse.de>
>> 
>> 	PR middle-end/80222
>> 	* gimple-fold.c (gimple_fold_indirect_ref): Do not touch
>> 	TYPE_REF_CAN_ALIAS_ALL references.
>> 	* fold-const.c (fold_indirect_ref_1): Likewise.
>
>You just backported it onto the 6 branch and the fold_indirect_ref_1
>hunk 
>alone introduced regressions in the C testsuite on the branch for
>strict-
>alignment architectures, e.g. SPARC.  Reduced testcase:
>
>typedef long long V
>__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>
>typedef struct S { V b; } P __attribute__((aligned (1)));
>
>__attribute__((noinline, noclone)) void
>bar (P *p)
>{
>  p->b[1] = 5;
>}
>
>Before the patch at -O0:
>
>bar:
>        save    %sp, -96, %sp
>        st      %i0, [%fp+68]
>        ld      [%fp+68], %g1
>        ldub    [%g1+12], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+12]
>        ldub    [%g1+13], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+13]
>        ldub    [%g1+14], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+14]
>        ldub    [%g1+15], %g2
>        and     %g2, 0, %g2
>        or      %g2, 5, %g2
>        stb     %g2, [%g1+15]
>        ldub    [%g1+8], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+8]
>        ldub    [%g1+9], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+9]
>        ldub    [%g1+10], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+10]
>        ldub    [%g1+11], %g2
>        and     %g2, 0, %g2
>        stb     %g2, [%g1+11]
>        nop
>        return  %i7+8
>         nop
>
>
>After the patch:
>
>bar:
>        save    %sp, -96, %sp
>        st      %i0, [%fp+68]
>        ld      [%fp+68], %g1
>        add     %g1, 8, %g1
>        mov     0, %g2
>        mov     5, %g3
>        std     %g2, [%g1]
>        nop
>        return  %i7+8
>         nop
>
>that is to say, the compiler now generates an unaligned store.

Does this happen on the GCC7 branch as well?  The patch just guards an indirect ref folding (I refrained from trying to make it correct given I think it's premature optimization).

I'll try to investigate on Monday if you don't beat me to it.  Feel free to revert the backport in the meantime.

Note I think you can trigger the same bug with some source changes independent of the patch which means the GENERIC must be somehow invalid.

Thanks,
Richard.

Re: [PATCH] Fix PR80222

[Eric Botcazou]

> Does this happen on the GCC7 branch as well?  The patch just guards an
> indirect ref folding (I refrained from trying to make it correct given I
> think it's premature optimization).

No, mainline and GCC 7 branch are fine.  It appears that the folding (probably 
to BIT_FIELD_REF) is necessary to break apart the store on the 6 branch.

> I'll try to investigate on Monday if you don't beat me to it.  Feel free to
> revert the backport in the meantime.

No urgency, it's rather marginal.

> Note I think you can trigger the same bug with some source changes
> independent of the patch which means the GENERIC must be somehow invalid.

Possibly so, yes.

-- 
Eric Botcazou

Re: [PATCH] Fix PR80222

[Richard Biener]

On Sat, 13 May 2017, Eric Botcazou wrote:

> > Does this happen on the GCC7 branch as well?  The patch just guards an
> > indirect ref folding (I refrained from trying to make it correct given I
> > think it's premature optimization).
> 
> No, mainline and GCC 7 branch are fine.  It appears that the folding (probably 
> to BIT_FIELD_REF) is necessary to break apart the store on the 6 branch.
> 
> > I'll try to investigate on Monday if you don't beat me to it.  Feel free to
> > revert the backport in the meantime.
> 
> No urgency, it's rather marginal.
> 
> > Note I think you can trigger the same bug with some source changes
> > independent of the patch which means the GENERIC must be somehow invalid.
> 
> Possibly so, yes.

So on the branch the frontends generate

  *((long long int * {ref-all}) &p->b + 8) = 5;

that's bogus to the effect that the dereference happens in type
'long long int' rather than an unaligned variant of it.  On the
GCC 7 branch and trunk we now generate

  VIEW_CONVERT_EXPR<long long int[2]>(p->b)[1] = 5;

which does not introduce an artificial dereference and does the folding
(even for variable indices) directly here.  Which also hints at that
we mishandle

typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V b; } P __attribute__((aligned (1)));

__attribute__((noinline, noclone)) void
bar (P *p, int i)
{
  p->b[i] = 5;
}

even before the patch:

  *((long long int * {ref-all}) &p->b + (sizetype) ((unsigned int) i * 8)) 
= 5;

which cannot be folded into a BIT_FIELD_REF but we end up with

  V * {ref-all} D.1412;
  unsigned int i.0;
  unsigned int D.1414;
  long long int * {ref-all} D.1415;

  D.1412 = &p->b;
  i.0 = (unsigned int) i;
  D.1414 = i.0 * 8;
  D.1415 = D.1412 + D.1414;
  *D.1415 = 5;

and

bar:
        save    %sp, -96, %sp
        st      %i0, [%fp+68]
        st      %i1, [%fp+72]
        ld      [%fp+68], %g2
        ld      [%fp+72], %g1
        sll     %g1, 3, %g1
        add     %g2, %g1, %g1
        mov     0, %g2
        mov     5, %g3
        std     %g2, [%g1]
        nop
        restore
        jmp     %o7+8

which (not knowing SPARC assembly) looks bogus as well.

At this point I'll favor simply reverting the patch from the branch
rather than trying to backport the IL change for GENERIC.

Thus consider that done.

Richard.