[Bug tree-optimization/52395] New: [4.7 Regression] Alignment issue at O2

[Bug tree-optimization/52395] New: [4.7 Regression] Alignment issue at O2

[izamyatin at gmail dot com]

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

             Bug #: 52395
           Summary: [4.7 Regression] Alignment issue at O2
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: izamyatin@gmail.com


Created attachment 26759
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26759
Test case for the issue

I noticed that after the following commit 

commit 25b3bbad6624357c1ac6a31e0be6fca37182fb78
Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Jan 27 14:56:54 2012 +0000

    2012-01-27  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/50444
        * tree-sra.c (build_ref_for_offset): Properly adjust the
        MEM_REF type for unaligned accesses.

        * gcc.dg/torture/pr50444.c: New testcase.


    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@183630
138bc75d-0d04-0410-961f-82ee72b054a4

 alignment of some memory references was spoiled. For attached test case I see
following:

  before commit (A32):

#(insn:TI 6 11 7 2 (set (mem:SF (reg/v/f:DI 5 di [orig:61 __x ] [61]) [2
__x_1(D)->re+0 S4 A32])
#        (const_double:SF 0.0 [0x0.0p+0])) test.cpp:13 108 {*movsf_internal}
#     (nil))
    movl    $0x00000000, (%rdi)    # 6    *movsf_internal/5    [length = 16]


  after commit (A8):

#(insn:TI 6 11 7 2 (set (mem:SF (reg/v/f:DI 5 di [orig:61 __x ] [61]) [2
__x_1(D)->re+0 S4 A8])
#        (const_double:SF 0.0 [0x0.0p+0])) test.cpp:13 108 {*movsf_internal}
#     (nil))
    movl    $0x00000000, (%rdi)    # 6    *movsf_internal/5    [length = 16]


 Compiled just with O2.

 Originally I saw this after compilation of complex_io.cc from libstdc++-v3

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |x86_64-*-*
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2012-02-27
   Target Milestone|---                         |4.7.0
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 09:33:22 UTC ---
How is it "spoiled"?  It's now more conservative.  I suppose x86_64?

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[izamyatin at gmail dot com]

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

--- Comment #2 from Igor Zamyatin <izamyatin at gmail dot com> 2012-02-27 09:47:38 UTC ---
Right, more conservative :)

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[izamyatin at gmail dot com]

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

--- Comment #3 from Igor Zamyatin <izamyatin at gmail dot com> 2012-02-27 09:49:38 UTC ---
x86_64 indeed, sorry

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jamborm at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 10:00:09 UTC ---
Btw, the issue is

     ???  But it is not at all clear that prev_base is an access
     that was in the IL that way, so be conservative for now.

I can trivially eliminate one caller to build_ref_for_offset (from ipa-cp.c)
and privatize the routine to tree-sra.c, but IPA SRA callers remain and
those are the problematic ones I think.

Martin, can we make sure that 'base' as passed to build_ref_for_offset
is not artificially constructed by any of its callers?  Thus, that it is
at most the result of stripping some handled-component-refs from an
existing reference tree (that was not wrapped inside an ADDR_EXPR) in the IL?

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 10:03:10 UTC ---
Nearly all callers are, of course, via get_ref_for_model... so the question
holds the same for its 'base' parameter.

[Bug tree-optimization/52395] [4.7 Regression] Alignment issue at O2

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 10:25:49 UTC ---
If all was well ('base' is literally appearing in the IL before SRA / IPA-SRA)
we could do

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c      (revision 184591)
+++ gcc/tree-sra.c      (working copy)
@@ -1526,10 +1526,12 @@ build_ref_for_offset (location_t loc, tr
      we can extract more optimistic alignment information
      by looking at the access mode.  That would constrain the
      alignment of base + base_offset which we would need to
-     adjust according to offset.
-     ???  But it is not at all clear that prev_base is an access
-     that was in the IL that way, so be conservative for now.  */
+     adjust according to offset.  */
   align = get_pointer_alignment_1 (base, &misalign);
+  if (misalign == 0
+      && (TREE_CODE (prev_base) == MEM_REF
+         || TREE_CODE (prev_base) == TARGET_MEM_REF))
+    align = MAX (align, TYPE_ALIGN (TREE_TYPE (prev_base)));
   misalign += (double_int_sext (tree_to_double_int (off),
                                TYPE_PRECISION (TREE_TYPE (off))).low
               * BITS_PER_UNIT);

to mimic get_object_or_type_alignment.

At least for the ipa-cp.c use I _do_ know there is no such access.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|WAITING                     |NEW
            Summary|[4.7 Regression] Alignment  |[4.7 Regression] Too
                   |issue at O2                 |conservative alignment info
                   |                            |from SRA

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 10:48:37 UTC ---
Confirmed at least.  The regression is because of a wrong-code fix.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 13:09:12 UTC ---
The main issue is, of course, that we re-write base of the LHS of the
assignments at all.

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c      (revision 184591)
+++ gcc/tree-sra.c      (working copy)
@@ -1480,6 +1480,10 @@ build_ref_for_offset (location_t loc, tr

   gcc_checking_assert (offset % BITS_PER_UNIT == 0);

+  if (offset == 0
+      && types_compatible_p (exp_type, TREE_TYPE (base)))
+    return unshare_expr (base);
+
   base = get_addr_base_and_unit_offset (base, &base_offset);

   /* get_addr_base_and_unit_offset returns NULL for references with a variable

or, instead of re-writing

  *__x_1(D) = D.2219;

to

  __x_1(D)->re = SR.1_11;
  __x_1(D)->im = SR.2_10;

re-write it to

  SR.3_12 = COMPLEX_EXPR <SR.1_11, SR.2_12>;
  *__x_1(D) = SR.3_12;

to avoid touching the non-replaced LHS.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-27 14:51:27 UTC ---
(In reply to comment #6)
> If all was well ('base' is literally appearing in the IL before SRA / IPA-SRA)
> we could do
> 
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 184591)
> +++ gcc/tree-sra.c      (working copy)
> @@ -1526,10 +1526,12 @@ build_ref_for_offset (location_t loc, tr
>       we can extract more optimistic alignment information
>       by looking at the access mode.  That would constrain the
>       alignment of base + base_offset which we would need to
> -     adjust according to offset.
> -     ???  But it is not at all clear that prev_base is an access
> -     that was in the IL that way, so be conservative for now.  */
> +     adjust according to offset.  */
>    align = get_pointer_alignment_1 (base, &misalign);
> +  if (misalign == 0
> +      && (TREE_CODE (prev_base) == MEM_REF
> +         || TREE_CODE (prev_base) == TARGET_MEM_REF))
> +    align = MAX (align, TYPE_ALIGN (TREE_TYPE (prev_base)));
>    misalign += (double_int_sext (tree_to_double_int (off),
>                                 TYPE_PRECISION (TREE_TYPE (off))).low
>                * BITS_PER_UNIT);
> 
> to mimic get_object_or_type_alignment.
> 
> At least for the ipa-cp.c use I _do_ know there is no such access.

This one bootstrapped and tested ok on x86_64.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[jamborm at gcc dot gnu.org]

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

--- Comment #10 from Martin Jambor <jamborm at gcc dot gnu.org> 2012-02-27 16:25:15 UTC ---
(In reply to comment #4)
> Martin, can we make sure that 'base' as passed to build_ref_for_offset
> is not artificially constructed by any of its callers?  Thus, that it is
> at most the result of stripping some handled-component-refs from an
> existing reference tree (that was not wrapped inside an ADDR_EXPR) in the IL?

As you noticed in PR 52402 IPA-SRA code does not use this function and
IPA-CP only does so when the address argument is a global declaration,
which should be always handled well by expand, I hope.

However, I believe it's the plain ordinary SRA which can introduce,
accesses which the base did not have before.  Consider function foo in
the following testcase:

--------------------------------------------------
typedef long long __m128i __attribute__ ((__vector_size__ (16),
                      __may_alias__));
typedef unsigned int uint32_t;
extern void abort ();

typedef struct
{
  __m128i b;
  int a;
} s_1a;

typedef s_1a s_1m __attribute__((aligned(1)));

typedef struct
{
  char c;
  s_1m s;
} __attribute__((packed)) s_2;

void __attribute__((noinline, noclone))
foo (s_1m *p)
{
  __m128i v1 = {1,1};
  s_1a l;

  l = *p;
  l.b = v1;
  *p = l;
}

int
main (int argc, char **argv)
{
  __m128i v2 = {3,4};
  long long z[2];
  s_2 x;


  x.s.b = v2;
  foo (&x.s);
  __builtin_memcpy (&z, &x.s.b, sizeof (x.s.b));

  if (z[0] != 1 || z[1] != 1)
    abort ();

  return 0;
}
--------------------------------------------------

The original foo:

  l = *p_2(D);
  l.b = { 1, 1 };
  *p_2(D) = l;

is basically converted into (optimized dump):

  p_2(D)->b = { 1, 1 };

so we have introduced a non-BLKmode access into a base which did not
have it previously.  I am not sure whether this fits your definition
of base which was already there or not.  On the other hand, this
testcase passes with the patch from comment 6 (but fails with PR 50444
fix reverted) and SRA never creates anything more beastly than this,
i.e. forwarding scalar loads/stores across aggregate assignments.

Does that answer your question?

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[jamborm at gcc dot gnu.org]

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

--- Comment #11 from Martin Jambor <jamborm at gcc dot gnu.org> 2012-02-27 17:15:42 UTC ---
(In reply to comment #8)
> The main issue is, of course, that we re-write base of the LHS of the
> assignments at all.
> 
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 184591)
> +++ gcc/tree-sra.c      (working copy)
> @@ -1480,6 +1480,10 @@ build_ref_for_offset (location_t loc, tr
> 
>    gcc_checking_assert (offset % BITS_PER_UNIT == 0);
> 
> +  if (offset == 0
> +      && types_compatible_p (exp_type, TREE_TYPE (base)))
> +    return unshare_expr (base);
> +

I thought about this too.  However, at that time my plans also
included rolling back build_ref_for_model to only construct
COMPONENT_REFs for bit-fields (or get rid of it altogether if we can
ever handle bit-fields better) and thus this situation would not occur
as often as now (only with the bit-fields, really). Also, it would not
solve all the misalignment cases, and so I did not proceed with it.
But if build_ref_for_model is going to stay, if we want to preserve
COMPONENT_REFs over SRA, then I suppose we want this hunk as well.

> or, instead of re-writing
> 
>   *__x_1(D) = D.2219;
> 
> to
> 
>   __x_1(D)->re = SR.1_11;
>   __x_1(D)->im = SR.2_10;
> 
> re-write it to
> 
>   SR.3_12 = COMPLEX_EXPR <SR.1_11, SR.2_12>;
>   *__x_1(D) = SR.3_12;
> 
> to avoid touching the non-replaced LHS.

Well, we would sort-of-do-this if the aggregate contained a gcc
internal complex type but these are two floats within a structure,
from the point of view of SRA, at least.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenther at suse dot de]

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

--- Comment #12 from rguenther at suse dot de <rguenther at suse dot de> 2012-02-28 09:02:04 UTC ---
On Mon, 27 Feb 2012, jamborm at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52395
> 
> --- Comment #10 from Martin Jambor <jamborm at gcc dot gnu.org> 2012-02-27 16:25:15 UTC ---
> (In reply to comment #4)
> > Martin, can we make sure that 'base' as passed to build_ref_for_offset
> > is not artificially constructed by any of its callers?  Thus, that it is
> > at most the result of stripping some handled-component-refs from an
> > existing reference tree (that was not wrapped inside an ADDR_EXPR) in the IL?
> 
> As you noticed in PR 52402 IPA-SRA code does not use this function and
> IPA-CP only does so when the address argument is a global declaration,
> which should be always handled well by expand, I hope.
> 
> However, I believe it's the plain ordinary SRA which can introduce,
> accesses which the base did not have before.  Consider function foo in
> the following testcase:
> 
> --------------------------------------------------
> typedef long long __m128i __attribute__ ((__vector_size__ (16),
>                       __may_alias__));
> typedef unsigned int uint32_t;
> extern void abort ();
> 
> typedef struct
> {
>   __m128i b;
>   int a;
> } s_1a;
> 
> typedef s_1a s_1m __attribute__((aligned(1)));
> 
> typedef struct
> {
>   char c;
>   s_1m s;
> } __attribute__((packed)) s_2;
> 
> void __attribute__((noinline, noclone))
> foo (s_1m *p)
> {
>   __m128i v1 = {1,1};
>   s_1a l;
> 
>   l = *p;
>   l.b = v1;
>   *p = l;
> }
> 
> int
> main (int argc, char **argv)
> {
>   __m128i v2 = {3,4};
>   long long z[2];
>   s_2 x;
> 
> 
>   x.s.b = v2;
>   foo (&x.s);
>   __builtin_memcpy (&z, &x.s.b, sizeof (x.s.b));
> 
>   if (z[0] != 1 || z[1] != 1)
>     abort ();
> 
>   return 0;
> }
> --------------------------------------------------
> 
> The original foo:
> 
>   l = *p_2(D);
>   l.b = { 1, 1 };
>   *p_2(D) = l;
> 
> is basically converted into (optimized dump):
> 
>   p_2(D)->b = { 1, 1 };
> 
> so we have introduced a non-BLKmode access into a base which did not
> have it previously.  I am not sure whether this fits your definition
> of base which was already there or not.  On the other hand, this
> testcase passes with the patch from comment 6 (but fails with PR 50444
> fix reverted) and SRA never creates anything more beastly than this,
> i.e. forwarding scalar loads/stores across aggregate assignments.
> 
> Does that answer your question?

Yes, I think so.

Richard.

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-28 09:18:38 UTC ---
Author: rguenth
Date: Tue Feb 28 09:18:35 2012
New Revision: 184620

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184620
Log:
2012-02-28  Richard Guenther  <rguenther@suse.de>

    PR tree-optimization/52395
    * tree-sra.c (build_ref_for_offset): Also look at the base
    TYPE_ALIGN when figuring out the alignment of the replacement.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-sra.c

[Bug tree-optimization/52395] [4.7 Regression] Too conservative alignment info from SRA

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #14 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-28 09:19:27 UTC ---
Fixed.