[Bug target/50494] New: gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[Bug target/50494] New: gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[vries at gcc dot gnu.org]

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

             Bug #: 50494
           Summary: gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc
                    with -flto
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: vries@gcc.gnu.org


The testcase contains 2 char array initializers:
...
__attribute__ ((noinline))
void main1 (signed char x, signed char max_result, signed char min_result)
{
  int i;

  signed char b[N] = {1,2,3,6,8,10,12,14,16,18,20,22,24,26,28,30};
  signed char c[N] = {1,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
...

The initializer data is put into .rodata, and copied to stack at the start of
the function by 2 blockmoves.

When we do expand_block_move during cc1, the blockmove source looks like this
as a tree:
...
 <var_decl 0xf7e37180 *.LC0
    type <array_type 0xf7e317e0
        type <integer_type 0xf7d53180 signed char sizes-gimplified public
string-flag QI
            size <integer_cst 0xf7d406c8 constant 8>
            unit size <integer_cst 0xf7d406e4 constant 1>
            align 8 symtab 0 alias set 0 canonical type 0xf7d53180 precision 8
min <integer_cst 0xf7d40674 -128> max <integer_cst 0xf7d406ac 127>
            pointer_to_this <pointer_type 0xf7e31840>>
        sizes-gimplified BLK
        size <integer_cst 0xf7d409f4 constant 128>
        unit size <integer_cst 0xf7d40a10 constant 16>
        align 8 symtab 0 alias set 0 canonical type 0xf7e317e0
        domain <integer_type 0xf7e1ff00 type <integer_type 0xf7d53000 sizetype>
            sizes-gimplified SI
            size <integer_cst 0xf7d40508 constant 32>
            unit size <integer_cst 0xf7d40524 constant 4>
            align 32 symtab 0 alias set -1 canonical type 0xf7e1ff00 precision
32 min <integer_cst 0xf7d40540 0> max <integer_cst 0xf7e1c524 15>>
        pointer_to_this <pointer_type 0xf7e37780>>
    readonly used static ignored in-constant-pool BLK file (null) line 0 col 0
size <integer_cst 0xf7d409f4 128> unit size <integer_cst 0xf7d40a10 16>
    user align 128 initial <constructor 0xf7db5860>
    (mem/s/c:BLK (symbol_ref/f:SI ("*.LC0") [flags 0x82] <var_decl 0xf7e37180
*.LC0>) [1 S16 A8])>
...

and like this as rtl:
...
(mem/s/c:BLK (reg/f:SI 180) [1 S16 A8])
...

This case is chosen in expand_block_move, and the blockmoves are expanded as 
4-byte wordmoves.
...
      else if (bytes >= 4 && (align >= 32 || !STRICT_ALIGNMENT))
    {            /* move 4 bytes */
      move_bytes = 4;
      mode = SImode;
      gen_func.mov = gen_movsi;
    }
...

The .rodata section written by cc1 has align 2^4 == 16 bytes.
...
    .section    .rodata
    .align 4
    .set    .LANCHOR0,. + 0
.LC0:
    .byte    1
    .byte    2
...


However, when we do expand_block_move during lto1, the blockmove source looks
like this as a tree:
...
 <mem_ref 0xf7dc76c8
    type <array_type 0xf7dc67e0
        type <integer_type 0xf7d51180 signed char public string-flag QI
            size <integer_cst 0xf7d4071c constant 8>
            unit size <integer_cst 0xf7d40738 constant 1>
            align 8 symtab 0 alias set -1 canonical type 0xf7d51180 precision 8
min <integer_cst 0xf7d406ac -128> max <integer_cst 0xf7d40700 127>
            pointer_to_this <pointer_type 0xf7dce8a0>>
        BLK
        size <integer_cst 0xf7d40a48 constant 128>
        unit size <integer_cst 0xf7d40a64 constant 16>
        align 8 symtab 0 alias set 0 canonical type 0xf7dc67e0
        domain <integer_type 0xf7dc6840 type <integer_type 0xf7d51000 sizetype>
            SI
            size <integer_cst 0xf7d40524 constant 32>
            unit size <integer_cst 0xf7d40540 constant 4>
            align 32 symtab 0 alias set -1 canonical type 0xf7d51360 precision
32 min <integer_cst 0xf7d4055c 0> max <integer_cst 0xf7dc7118 15>>
        pointer_to_this <pointer_type 0xf7dc6a20>>
    readonly
    arg 0 <addr_expr 0xf7dc0a38
        type <pointer_type 0xf7dc6a20 type <array_type 0xf7dc67e0>
            public unsigned SI size <integer_cst 0xf7d40524 32> unit size
<integer_cst 0xf7d40540 4>
            align 32 symtab 0 alias set -1 canonical type 0xf7dc6a20>
        readonly constant
        arg 0 <var_decl 0xf7dc6780 *.LC0 type <array_type 0xf7dc67e0>
            readonly used static ignored in-constant-pool BLK file (null) line
0 col 0 size <integer_cst 0xf7d40a48 128> unit size <integer_cst 0xf7d40a64 16>
            user align 128 initial <constructor 0xf7dc51f0>
            (mem/s/c:BLK (symbol_ref/f:SI ("*.LC0") [flags 0x82] <var_decl
0xf7dd9060 *.LC0>) [1 S16 A8])>>
    arg 1 <integer_cst 0xf7dc76e4 type <pointer_type 0xf7dc6a20> constant 0>
   
/scratch/vries/b6/pr43864.42.all-fsf-mainline-powerpc-linux-gnu.cfg/src/gcc-mainline/gcc/testsuite/gcc.dg/vect/vect-reduc-2char.c:11:15>
...

and like this as rtx:
...
(mem/s/u/c:BLK (reg/f:SI 180) [0 *.LC0+0 S16 A128])
...

This case is now chosen in expand_block_move, and the blockmoves are expanded
as 16-byte vectormoves.
...
      if (TARGET_ALTIVEC && bytes >= 16 && align >= 128)
    {
      move_bytes = 16;
      mode = V4SImode;
      gen_func.mov = gen_movv4si;
    }
...

The .rodata section written by lto1 however now has align 2^2 == 4 bytes.
...
    .section    .rodata
    .align 2
    .set    .LANCHOR0,. + 0
.LC0:
    .byte    1
    .byte    2
...

This alignment is not enough for the vector moves which require 16-byte
alignment. This causes the test to fail spuriously.

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[vries at gcc dot gnu.org]

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

--- Comment #1 from vries at gcc dot gnu.org 2011-09-23 14:52:55 UTC ---
Created attachment 25348
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25348
test.c minimized from gcc.dg/vect/vect-reduc-2char.c

To reproduce:

$ powerpc-linux-gnu-gcc -O2 -ftree-vectorize test.c -flto -maltivec -static
$ ./a.out ; echo $?
218

The problematic code is still generated without -static, but for me this
minimal testcase does not fail runtime without the -static.

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[vries at gcc dot gnu.org]

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

--- Comment #2 from vries at gcc dot gnu.org 2011-09-23 15:05:19 UTC ---
Same issue occurs for gcc.dg/vect/pr44507.c with -m64.

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[vries at gcc dot gnu.org]

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

--- Comment #3 from vries at gcc dot gnu.org 2011-09-23 22:12:13 UTC ---
Configure line for compiler same as
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50499#c2

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[irar at il dot ibm.com]

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

Ira Rosen <irar at il dot ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-01-02
                 CC|                            |irar at il dot ibm.com
     Ever Confirmed|0                           |1

--- Comment #4 from Ira Rosen <irar at il dot ibm.com> 2012-01-02 07:01:10 UTC ---
I see on powerpc64-suse-linux:

FAIL: gcc.dg/vect/pr44507.c -flto execution test
FAIL: gcc.dg/vect/pr45752.c -flto execution test
FAIL: gcc.dg/vect/vect-reduc-2short.c -flto execution test
FAIL: gcc.dg/vect/vect-reduc-6.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-1.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-2.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-3.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-4.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-5.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-6.c -flto execution test
FAIL: gcc.dg/vect/slp-perm-7.c -flto execution test

Removing static array initialization fixes the failures.

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

--- Comment #5 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 18:13:45 UTC ---
Created attachment 29426
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29426
Assembly file of slp-perm-1.c after lto with -mcpu=power6 -O3 -maltivec

[Bug target/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

--- Comment #6 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 18:16:28 UTC ---
Created attachment 29427
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29427
slp-perm-1.c assembly file before LTO is run with -mcpu=power6 -O3 -maltivec

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

Michael Meissner <meissner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |lto

--- Comment #7 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 18:25:38 UTC ---
I am switching this to LTO instead of target, as it appears to be an LTO bug. 
Before LTO is run, the alignment of the .rodata section is 16 byte alignment
since the array used to initialize the auto array is copied with altivec
instructions.  After LTO, the alignment of the .rodata section is 4 bytes.  The
powerpc Altivec instructions ignore the bottom 4 bits of the address, and so
depending on what else is linked, the test will randomly fail or succeed.

I added attachments from compiling slp-perm-1.c with -O3 -mcpu=power6 -maltivec
-save-temps to give the asm files.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-12 18:36:12 UTC ---
I will have a looksee tomorrow.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

--- Comment #9 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 19:07:18 UTC ---
The -fsection-anchors option appears to be important.  If I use
-fsection-anchors (which is default for powerpc64-linux), LTO does not align
the .rodata section, but uses Altivec memory instructions.  If I use
-fno-section-anchors, the .rodata section is not aligned, but it doesn't use
Altivec memory instructions, so the test passes.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

--- Comment #10 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 19:16:56 UTC ---
If -fno-merge-constants (and the default -fsection-anchors) is used, then the
correct alignment for the table is set (and Altivec memory instructions are
used).

At a guess, it is likely be in the gimplify_init_constructor function in
gimplify.c.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> 2013-02-13 09:05:16 UTC ---
On Tue, 12 Feb 2013, meissner at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #10 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-12 19:16:56 UTC ---
> If -fno-merge-constants (and the default -fsection-anchors) is used, then the
> correct alignment for the table is set (and Altivec memory instructions are
> used).
> 
> At a guess, it is likely be in the gimplify_init_constructor function in
> gimplify.c.

There is special code to handle initializers, it may be well that with
LTO we fail to do the correct thing here.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-13 10:55:15 UTC ---
Created attachment 29436
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29436
patch

-fsection-anchors enables pass_ipa_increase_alignment (the vectorizer can
not align globals after any function was assembled, so we do that upfront).

In LTO:

  /* If this variable belongs to the global constant pool, retrieve the
     pre-computed RTL or recompute it in LTO mode.  */
  if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
    {
      SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
      return;

but obviously output_constant_def when just getting DECL_INITIAL cannot
honor any special alignment requirements of decl.  It will simply get
a new decl with standard alignment.

We know decl is already the decl associated with the constant, so we
should just re-use it.

So I believe the attached should fix this.  Can PPC people plaease test?

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-13 10:55:59 UTC ---
Eric added the original feature.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-13 22:13:42 UTC ---
> In LTO:
> 
>   /* If this variable belongs to the global constant pool, retrieve the
>      pre-computed RTL or recompute it in LTO mode.  */
>   if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
>     {
>       SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
>       return;
> 
> but obviously output_constant_def when just getting DECL_INITIAL cannot
> honor any special alignment requirements of decl.  It will simply get
> a new decl with standard alignment.
> 
> We know decl is already the decl associated with the constant, so we
> should just re-use it.

Why does the alignment of a DECL_IN_CONSTANT_POOL matter here exactly?

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[meissner at gcc dot gnu.org]

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

--- Comment #15 from Michael Meissner <meissner at gcc dot gnu.org> 2013-02-13 22:38:12 UTC ---
The patch does align the .rodata section to 16 byte alignment, but the code to
load up the auto vector from constant memory does not do vectorization.

If I use -fno-section-anchors, it aligns .rodata to 4 byte alignment, and does
not vectorize the code.

If I use -fno-merge-constants, it aligns .rodata to 16 byte alignment, and does
vectorize the code.

If I use -fno-merge-constants without -flto, it aligns .rodata to 16 byte
alignment, but it uses unaligned vector loads/stores.

So the patch does help in that the tests now pass that were randomly failing.

While it would be nice if we could get the initialization to be vectorized, I'm
not how performance critical this is.

Eric: if the alignment of the constant data that is used to initialize the auto
array is a mismatch, and you use Altivec instructions, when the compiler
auto-vectorizes the copy, the wrong data gets used.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #16 from rguenther at suse dot de <rguenther at suse dot de> 2013-02-14 08:56:12 UTC ---
On Wed, 13 Feb 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-13 22:13:42 UTC ---
> > In LTO:
> > 
> >   /* If this variable belongs to the global constant pool, retrieve the
> >      pre-computed RTL or recompute it in LTO mode.  */
> >   if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
> >     {
> >       SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
> >       return;
> > 
> > but obviously output_constant_def when just getting DECL_INITIAL cannot
> > honor any special alignment requirements of decl.  It will simply get
> > a new decl with standard alignment.
> > 
> > We know decl is already the decl associated with the constant, so we
> > should just re-use it.
> 
> Why does the alignment of a DECL_IN_CONSTANT_POOL matter here exactly?

Because as it is seen as a regular VAR_DECL by optimizers the
vectorizer (well, IPA increase-alignment in this case) chooses to
bump its alignment to be able to use aligned vector moves.

Thus we need to honor the increased DECL_ALIGN when outputting the
constant pool, otherwise we generate wrong-code.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #17 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-14 09:18:46 UTC ---
> Because as it is seen as a regular VAR_DECL by optimizers the
> vectorizer (well, IPA increase-alignment in this case) chooses to
> bump its alignment to be able to use aligned vector moves.
> 
> Thus we need to honor the increased DECL_ALIGN when outputting the
> constant pool, otherwise we generate wrong-code.

I see, thanks.  Perhaps the safest solution is to prevent IPA from increasing
the alignment if DECL_IN_CONSTANT_POOL, as initializations of aggregate objects
are presumably not supposed to happen in hot spots.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #18 from rguenther at suse dot de <rguenther at suse dot de> 2013-02-14 10:41:07 UTC ---
On Thu, 14 Feb 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #17 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-02-14 09:18:46 UTC ---
> > Because as it is seen as a regular VAR_DECL by optimizers the
> > vectorizer (well, IPA increase-alignment in this case) chooses to
> > bump its alignment to be able to use aligned vector moves.
> > 
> > Thus we need to honor the increased DECL_ALIGN when outputting the
> > constant pool, otherwise we generate wrong-code.
> 
> I see, thanks.  Perhaps the safest solution is to prevent IPA from increasing
> the alignment if DECL_IN_CONSTANT_POOL, as initializations of aggregate objects
> are presumably not supposed to happen in hot spots.

It's also done by the vectorizer pass (for -fno-section-anchors).
I believe it's wrong to not honor DECL_ALIGN of decl in

  /* If this variable belongs to the global constant pool, retrieve the
     pre-computed RTL or recompute it in LTO mode.  */
  if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
    {
      SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
      return;

which is what happens here.  Thus, if we say DECL_IN_CONSTANT_POOL
decls may not have their alignment changed we should assert that
(but my patch just honors DECL_ALIGN and avoids creation of yet
another DECL_IN_CONSTANT_POOL decl ...).  After all we also
use DECL_ALIGN information if anybody inspects the address
(which may happen if we elide the local static to the initializer
if we can see it is never changed - I suppose we cannot do that
at the moment)

Btw, if properly aligned the block-move can use vector code as well
(not sure if any target does that though).

Richard.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

--- Comment #19 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-14 12:24:26 UTC ---
Author: rguenth
Date: Thu Feb 14 12:24:12 2013
New Revision: 196050

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196050
Log:
2013-02-14  Richard Biener  <rguenther@suse.de>

    PR lto/50494
    * varasm.c (output_constant_def_1): Get the decl representing
    the constant as argument.
    (output_constant_def): Wrap output_constant_def_1.
    (make_decl_rtl): Use output_constant_def_1 with the decl
    representing the constant.
    (build_constant_desc): Optionally re-use a decl already
    representing the constant.
    (tree_output_constant_def): Adjust.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/varasm.c

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.8.0

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-14 12:25:02 UTC ---
Fixed for 4.8.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

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

--- Comment #21 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 15:30:50 UTC ---
Reopened, the fix breaks LTO bootstrap with Ada:

/tmp/cc5Nhl9J.s: Assembler messages:
/tmp/cc5Nhl9J.s:6206: Error: symbol `.LC0' is already defined
/tmp/cc5Nhl9J.s:6219: Error: symbol `.LC0' is already defined
/tmp/cc5Nhl9J.s:6996: Error: symbol `.LC0' is already defined
make[4]: *** [/tmp/ccuFb8kg.ltrans2.ltrans.o] Error 1
make[4]: *** Waiting for unfinished jobs....
/tmp/ccf05Tt7.s: Assembler messages:
/tmp/ccf05Tt7.s:5618: Error: symbol `.LC5' is already defined
/tmp/ccf05Tt7.s:5629: Error: symbol `.LC5' is already defined
make[4]: *** [/tmp/ccuFb8kg.ltrans8.ltrans.o] Error 1
lto-wrapper: make returned 2 exit status
/home/eric/install/gcc/x86_64-suse-linux/bin/ld: lto-wrapper failed
collect2: error: ld returned 1 exit status
make[3]: *** [gnatbind] Error 1
make[3]: *** Waiting for unfinished jobs....

The reason for the name collision seems obvious enough...  I still think the
best solution is not to fiddle with the alignment of DECL_IN_CONSTANT_POOL
objects at this point, since the machinery assumes that their alignment doesn't
matter (or more precisely that they only need to have the alignment of their
type, which is correct as far as I know).

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org

--- Comment #22 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-04 15:42:18 UTC ---
(In reply to comment #21)
> Reopened, the fix breaks LTO bootstrap with Ada:
> 
> /tmp/cc5Nhl9J.s: Assembler messages:
> /tmp/cc5Nhl9J.s:6206: Error: symbol `.LC0' is already defined
> /tmp/cc5Nhl9J.s:6219: Error: symbol `.LC0' is already defined
> /tmp/cc5Nhl9J.s:6996: Error: symbol `.LC0' is already defined
> make[4]: *** [/tmp/ccuFb8kg.ltrans2.ltrans.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> /tmp/ccf05Tt7.s: Assembler messages:
> /tmp/ccf05Tt7.s:5618: Error: symbol `.LC5' is already defined
> /tmp/ccf05Tt7.s:5629: Error: symbol `.LC5' is already defined
> make[4]: *** [/tmp/ccuFb8kg.ltrans8.ltrans.o] Error 1
> lto-wrapper: make returned 2 exit status
> /home/eric/install/gcc/x86_64-suse-linux/bin/ld: lto-wrapper failed
> collect2: error: ld returned 1 exit status
> make[3]: *** [gnatbind] Error 1
> make[3]: *** Waiting for unfinished jobs....
> 
> The reason for the name collision seems obvious enough...  I still think the
> best solution is not to fiddle with the alignment of DECL_IN_CONSTANT_POOL
> objects at this point, since the machinery assumes that their alignment doesn't
> matter (or more precisely that they only need to have the alignment of their
> type, which is correct as far as I know).

How can the patch cause a name collision when all the patch does is
avoid creating a new decl...?  They are static and thus should be
mangled.

Well, and if we want a new decl just to re-assign a unique name here
then we want to copy over alignment information.  That is, LTO should
handle constant-pool entries by _not_ streaming the decl then.  Honza,
how are those supposed to make the symtab -> WPA -> LTRANS transition
anyway?

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #23 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-04 16:10:53 UTC ---
> How can the patch cause a name collision when all the patch does is
> avoid creating a new decl...?  They are static and thus should be
> mangled.

They clearly aren't.

> Well, and if we want a new decl just to re-assign a unique name here
> then we want to copy over alignment information.  That is, LTO should
> handle constant-pool entries by _not_ streaming the decl then.  Honza,
> how are those supposed to make the symtab -> WPA -> LTRANS transition
> anyway?

The irony being that the initial implementation didn't stream the DECL but was
changed because the varpool was just starting to being streamed as well. :-)

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

--- Comment #24 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-05 10:18:01 UTC ---
(In reply to comment #23)
> > How can the patch cause a name collision when all the patch does is
> > avoid creating a new decl...?  They are static and thus should be
> > mangled.
> 
> They clearly aren't.
> 
> > Well, and if we want a new decl just to re-assign a unique name here
> > then we want to copy over alignment information.  That is, LTO should
> > handle constant-pool entries by _not_ streaming the decl then.  Honza,
> > how are those supposed to make the symtab -> WPA -> LTRANS transition
> > anyway?
> 
> The irony being that the initial implementation didn't stream the DECL but was
> changed because the varpool was just starting to being streamed as well. :-)

Yes, we do refer to the in the IL so we need to stream them (in some way).

The question is why we don't hit lto-lang.c:lto_set_decl_assembler_name
mangling of !TREE_PUBLIC decls when streaming in the decl for the constant
pool entries (or when computing the assembler name at some point).
I suppose we never compute decl-assembler-name for the constant pool entries
but emit them in a special way?  At least build_constant_desc seems to
create a raw SYMBOL_RER using the raw created label?  But then, as we
don't stream the constant-descs, the RTL should refer to unique labels
(but DECL_NAME and the SYMBOL_REF symbol do not agree).

Which means that eventually the following would fix it:

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c        (revision 196451)
+++ gcc/varasm.c        (working copy)
@@ -3124,6 +3124,11 @@ build_constant_desc (tree exp, tree decl
       else
        align_variable (decl, 0);
     }
+  /* If we already had a decl for this constant pool entry adjust its
+     label to be unique within this translation unit and to make it
+     consistent with the symbol-ref symbol we use below.  */
+   else
+     DECL_NAME (decl) = get_identifier (label);

   /* Now construct the SYMBOL_REF and the MEM.  */
   if (use_object_blocks_p ())

can you check that?

Thanks.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

--- Comment #25 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-05 13:19:53 UTC ---
Btw, I cannot reproduce the issue with

t.c:

void bar (int *);
void foo (void)
{
  int a[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0 };
  bar (a);
}

t2.c

void bar (int *);
void baz (void)
{
  int a[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0,
      1, 2, 3, 4, 5, 6, 7, 8, 9, 0 };
  bar (a);
}

t3.c:

void bar (int *a) { __builtin_printf ("%d\n", a[25]); }
int main()
{
  foo ();
  baz ();
  return 0;
}

and

gcc t.c t2.c t3.c -flto

I see three .LC0 being streamed in at WPA stage, shipped to a single
LTRANS unit and there getting .LC0, .LC1, and .LC2 symbols by means
of the existing build_constant_desc (they have all .LCO DECL_NAME decls,
but hey - even that would be fixed by my suggested patch.

Trying LTO Ada bootstrap now.  But I really fail to see how it should
break.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #26 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 13:51:28 UTC ---
> The question is why we don't hit lto-lang.c:lto_set_decl_assembler_name
> mangling of !TREE_PUBLIC decls when streaming in the decl for the constant
> pool entries (or when computing the assembler name at some point).
> I suppose we never compute decl-assembler-name for the constant pool entries
> but emit them in a special way?

Right, see make_decl_rtl:

  /* If this variable belongs to the global constant pool, retrieve the
     pre-computed RTL or recompute it in LTO mode.  */
  if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
    {
      SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
                         decl, 1));
      return;
    }

> At least build_constant_desc seems to create a raw SYMBOL_RER using the raw
> created label?  But then, as we don't stream the constant-descs, the RTL 
> should refer to unique labels (but DECL_NAME and the SYMBOL_REF symbol do not
> agree).

I think the problem is that, with your patch, the DECLs are not unified when
they have the same DECL_INITIAL (decl), even if they have the same RTL in the
end.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #27 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 13:58:15 UTC ---
On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #26 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 13:51:28 UTC ---
> > The question is why we don't hit lto-lang.c:lto_set_decl_assembler_name
> > mangling of !TREE_PUBLIC decls when streaming in the decl for the constant
> > pool entries (or when computing the assembler name at some point).
> > I suppose we never compute decl-assembler-name for the constant pool entries
> > but emit them in a special way?
> 
> Right, see make_decl_rtl:
> 
>   /* If this variable belongs to the global constant pool, retrieve the
>      pre-computed RTL or recompute it in LTO mode.  */
>   if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
>     {
>       SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
>                          decl, 1));
>       return;
>     }
> 
> > At least build_constant_desc seems to create a raw SYMBOL_RER using the raw
> > created label?  But then, as we don't stream the constant-descs, the RTL 
> > should refer to unique labels (but DECL_NAME and the SYMBOL_REF symbol do not
> > agree).
> 
> I think the problem is that, with your patch, the DECLs are not unified when
> they have the same DECL_INITIAL (decl), even if they have the same RTL in the
> end.

Hmm, but when I use the same contents for the two arrays in my simple
testcase I do get only a single .LC0 output referenced from two places.
We will end up sharing the same RTL for both (unmerged) DECLs - but
I don't see how this can be a problem?  Maybe we fail to set
TREE_ASM_WRITTEN on the duplicate and output it anyway via other 
mechanisms?

But I can reproduce the Ada LTO bootstrap failure ...

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #28 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 14:23:19 UTC ---
> Hmm, but when I use the same contents for the two arrays in my simple
> testcase I do get only a single .LC0 output referenced from two places.
> We will end up sharing the same RTL for both (unmerged) DECLs - but
> I don't see how this can be a problem?  Maybe we fail to set
> TREE_ASM_WRITTEN on the duplicate and output it anyway via other 
> mechanisms?

We simply fail to set TREE_ASM_WRITTEN on the DECL_INITIAL (decl) because it is
not shared anymore.  So probably:

Index: varasm.c
===================================================================
--- varasm.c    (revision 196416)
+++ varasm.c    (working copy)
@@ -3112,7 +3112,6 @@ build_constant_desc (tree exp, tree decl
         LTO mode.  Instead we set the flag that will be recognized in
         make_decl_rtl.  */
       DECL_IN_CONSTANT_POOL (decl) = 1;
-      DECL_INITIAL (decl) = desc->value;
       /* ??? CONSTANT_ALIGNMENT hasn't been updated for vector types on most
         architectures so use DATA_ALIGNMENT as well, except for strings.  */
       if (TREE_CODE (exp) == STRING_CST)
@@ -3125,6 +3124,8 @@ build_constant_desc (tree exp, tree decl
        align_variable (decl, 0);
     }

+  DECL_INITIAL (decl) = desc->value;
+
   /* Now construct the SYMBOL_REF and the MEM.  */
   if (use_object_blocks_p ())
     {

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #29 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 14:26:00 UTC ---
On Tue, 5 Mar 2013, rguenther at suse dot de wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #27 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 13:58:15 UTC ---
> On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote:
> 
> > 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> > 
> > --- Comment #26 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 13:51:28 UTC ---
> > > The question is why we don't hit lto-lang.c:lto_set_decl_assembler_name
> > > mangling of !TREE_PUBLIC decls when streaming in the decl for the constant
> > > pool entries (or when computing the assembler name at some point).
> > > I suppose we never compute decl-assembler-name for the constant pool entries
> > > but emit them in a special way?
> > 
> > Right, see make_decl_rtl:
> > 
> >   /* If this variable belongs to the global constant pool, retrieve the
> >      pre-computed RTL or recompute it in LTO mode.  */
> >   if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
> >     {
> >       SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
> >                          decl, 1));
> >       return;
> >     }
> > 
> > > At least build_constant_desc seems to create a raw SYMBOL_RER using the raw
> > > created label?  But then, as we don't stream the constant-descs, the RTL 
> > > should refer to unique labels (but DECL_NAME and the SYMBOL_REF symbol do not
> > > agree).
> > 
> > I think the problem is that, with your patch, the DECLs are not unified when
> > they have the same DECL_INITIAL (decl), even if they have the same RTL in the
> > end.
> 
> Hmm, but when I use the same contents for the two arrays in my simple
> testcase I do get only a single .LC0 output referenced from two places.
> We will end up sharing the same RTL for both (unmerged) DECLs - but
> I don't see how this can be a problem?  Maybe we fail to set
> TREE_ASM_WRITTEN on the duplicate and output it anyway via other 
> mechanisms?

So in fact the issue must be that we _do_ share the constant pool
entry but not the decl we use to refer to it.  Which isn't very
easily fixable (well, it _is_ fixable, but ... )

OTOH in assemble_variable we properly use

  /* If this symbol belongs to the tree constant pool, output the constant
     if it hasn't already been written.  */
  if (TREE_CONSTANT_POOL_ADDRESS_P (symbol))
    {
      tree decl = SYMBOL_REF_DECL (symbol);
      if (!TREE_ASM_WRITTEN (DECL_INITIAL (decl)))
        output_constant_def_contents (symbol);
      return;
    }

so any constant pool entry shouldn't be emitted multiple times ...

> But I can reproduce the Ada LTO bootstrap failure ...

So we can revert the part of the patch that ends up not creating
a new decl but only transfer DECL_ALIGN.  But then we still don't
"merge" the decls we use to refer to the constants, so I wonder
how creating less decls can fix this issue at all ...

Note that merging constants but not decls also can end up
creating bogusly aligned constants.  In fact it seems to me
that we need to arrange for the LTO path

  /* If this variable belongs to the global constant pool, retrieve the
     pre-computed RTL or recompute it in LTO mode.  */
  if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
    {
      SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
                                                 decl, 1));
      return;
    }

to never share a constant pool entry ... :/

Bah.  A reduced testcase would be nice to have ...

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #30 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 14:39:00 UTC ---
> So we can revert the part of the patch that ends up not creating
> a new decl but only transfer DECL_ALIGN.  But then we still don't
> "merge" the decls we use to refer to the constants, so I wonder
> how creating less decls can fix this issue at all ...

That would be worse, DECL_ALIGN should _not_ be fiddled with for constant pool
entries in the first place since the constant/DECL_INITIAL is shared.

> Note that merging constants but not decls also can end up
> creating bogusly aligned constants.  In fact it seems to me
> that we need to arrange for the LTO path
> 
>   /* If this variable belongs to the global constant pool, retrieve the
>      pre-computed RTL or recompute it in LTO mode.  */
>   if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
>     {
>       SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
>                                                  decl, 1));
>       return;
>     }
> 
> to never share a constant pool entry ... :/

We should simply not touch DECL_IN_CONSTANT_POOL variables, since they are not
regular VAR_DECLs but only represent the underlying constant.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #31 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 15:09:06 UTC ---
On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #28 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 14:23:19 UTC ---
> > Hmm, but when I use the same contents for the two arrays in my simple
> > testcase I do get only a single .LC0 output referenced from two places.
> > We will end up sharing the same RTL for both (unmerged) DECLs - but
> > I don't see how this can be a problem?  Maybe we fail to set
> > TREE_ASM_WRITTEN on the duplicate and output it anyway via other 
> > mechanisms?
> 
> We simply fail to set TREE_ASM_WRITTEN on the DECL_INITIAL (decl) because it is
> not shared anymore.  So probably:

But in all places I found we check TREE_ASM_WRITTEN on DECL_INITIAL
of the SYMBOL_REF_DECL ...

So it must be pure luck that we survived LTO bootstrap before my
patch (as far as I understand things).  Before my patch we created
yet another decl for the constant pool entry.  With my patch
we will have one less (we still have the decls from the constant
pool entries that end up being shared in the LTRANS unit).

So in the end I can agree to that my patch doesn't really fix
the original issue fully.  So we can as well revert it and
instead avoid messing with alignment of the constant pool entries.

But then the underlying issue with multiple decls refering to
the same constant pool entry with LTO remains, it just happens
that it isn't triggered anymore.

> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 196416)
> +++ varasm.c    (working copy)
> @@ -3112,7 +3112,6 @@ build_constant_desc (tree exp, tree decl
>          LTO mode.  Instead we set the flag that will be recognized in
>          make_decl_rtl.  */
>        DECL_IN_CONSTANT_POOL (decl) = 1;
> -      DECL_INITIAL (decl) = desc->value;
>        /* ??? CONSTANT_ALIGNMENT hasn't been updated for vector types on most
>          architectures so use DATA_ALIGNMENT as well, except for strings.  */
>        if (TREE_CODE (exp) == STRING_CST)
> @@ -3125,6 +3124,8 @@ build_constant_desc (tree exp, tree decl
>         align_variable (decl, 0);
>      }
> 
> +  DECL_INITIAL (decl) = desc->value;
> +
>    /* Now construct the SYMBOL_REF and the MEM.  */
>    if (use_object_blocks_p ())
>      {

Hmm, maybe.  Then, why do we copy the constant in the first place ...

Thus,

Index: varasm.c
===================================================================
--- varasm.c    (revision 196462)
+++ varasm.c    (working copy)
@@ -3087,7 +3087,7 @@ build_constant_desc (tree exp, tree decl
   int labelno;

   desc = ggc_alloc_constant_descriptor_tree ();
-  desc->value = copy_constant (exp);
+  desc->value = exp;

   /* Propagate marked-ness to copied constant.  */
   if (flag_mudflap && mf_marked_p (exp))

should be an "equivalent" fix.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 15:32:15 UTC ---
> But in all places I found we check TREE_ASM_WRITTEN on DECL_INITIAL
> of the SYMBOL_REF_DECL ...

Nope, maybe_output_constant_def_contents has:

  rtx symbol = XEXP (desc->rtl, 0);
  tree exp = desc->value;

  if (flag_syntax_only)
    return;

  if (TREE_ASM_WRITTEN (exp))
    /* Already output; don't do it again.  */
    return;

so the DECL_INITIAL of the SYMBOL_REF_DECL must be desc->value.

> So it must be pure luck that we survived LTO bootstrap before my
> patch (as far as I understand things).  Before my patch we created
> yet another decl for the constant pool entry.  With my patch
> we will have one less (we still have the decls from the constant
> pool entries that end up being shared in the LTRANS unit).

We use LTO on heavy Ada applications with an unmodified 4.7 compiler (modulo a
patch for -g) so I don't think that luck has anything to do here.

> So in the end I can agree to that my patch doesn't really fix
> the original issue fully.  So we can as well revert it and
> instead avoid messing with alignment of the constant pool entries.

That would be my preference.

> Hmm, maybe.  Then, why do we copy the constant in the first place ...
> 
> Thus,
> 
> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 196462)
> +++ varasm.c    (working copy)
> @@ -3087,7 +3087,7 @@ build_constant_desc (tree exp, tree decl
>    int labelno;
> 
>    desc = ggc_alloc_constant_descriptor_tree ();
> -  desc->value = copy_constant (exp);
> +  desc->value = exp;
> 
>    /* Propagate marked-ness to copied constant.  */
>    if (flag_mudflap && mf_marked_p (exp))
> 
> should be an "equivalent" fix.

This call to copy_constant has been there for ages though. so a little bit of
archeology would probably be in order before removing it.

In the meantime, I've successfully bootstrapped my patchlet so we can also go
for it.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

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

--- Comment #33 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 15:48:03 UTC ---
On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 15:32:15 UTC ---
> > But in all places I found we check TREE_ASM_WRITTEN on DECL_INITIAL
> > of the SYMBOL_REF_DECL ...
> 
> Nope, maybe_output_constant_def_contents has:
> 
>   rtx symbol = XEXP (desc->rtl, 0);
>   tree exp = desc->value;
> 
>   if (flag_syntax_only)
>     return;
> 
>   if (TREE_ASM_WRITTEN (exp))
>     /* Already output; don't do it again.  */
>     return;
> 
> so the DECL_INITIAL of the SYMBOL_REF_DECL must be desc->value.

Ah, ok ... too many smart TREE_ASM_WRITTEN bits around ...

> > So it must be pure luck that we survived LTO bootstrap before my
> > patch (as far as I understand things).  Before my patch we created
> > yet another decl for the constant pool entry.  With my patch
> > we will have one less (we still have the decls from the constant
> > pool entries that end up being shared in the LTRANS unit).
> 
> We use LTO on heavy Ada applications with an unmodified 4.7 compiler (modulo a
> patch for -g) so I don't think that luck has anything to do here.

Fine, so the above might be the only issue.

> > So in the end I can agree to that my patch doesn't really fix
> > the original issue fully.  So we can as well revert it and
> > instead avoid messing with alignment of the constant pool entries.
> 
> That would be my preference.

Which of course pessimizes code generation and probably causes
some testsuite fallout for ppc (the original reported spurious
fails).

> > Hmm, maybe.  Then, why do we copy the constant in the first place ...
> > 
> > Thus,
> > 
> > Index: varasm.c
> > ===================================================================
> > --- varasm.c    (revision 196462)
> > +++ varasm.c    (working copy)
> > @@ -3087,7 +3087,7 @@ build_constant_desc (tree exp, tree decl
> >    int labelno;
> > 
> >    desc = ggc_alloc_constant_descriptor_tree ();
> > -  desc->value = copy_constant (exp);
> > +  desc->value = exp;
> > 
> >    /* Propagate marked-ness to copied constant.  */
> >    if (flag_mudflap && mf_marked_p (exp))
> > 
> > should be an "equivalent" fix.
> 
> This call to copy_constant has been there for ages though. so a little bit of
> archeology would probably be in order before removing it.

;)  Did that, it's there since forever - well, I traced it back to
the point we only deferred string constants:

 37459      jakub                 p = (struct deferred_string *)
 37459      jakub                     xmalloc (sizeof (struct 
deferred_string));
 37459      jakub 
 37459      jakub                 p->exp = copy_constant (exp);
 37459      jakub                 p->label = desc->label;

I'm LTO bootstrapping

  desc->value = decl ? exp : copy_constant (exp);

and doing a regular bootstrap with the copy_constant completely removed
at the moment.  Just curious ...

> In the meantime, I've successfully bootstrapped my patchlet so we can also go
> for it.

I'm fine with that.

As I concluded that the original fix didn't fix the alignment issue
(well, not for all possible cases at least) reverting the original
fix works for me as well.

I'm working on a patch to avoid re-aligning constant pool entries.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[ebotcazou at gcc dot gnu.org]

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

--- Comment #34 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 16:05:44 UTC ---
Created attachment 29587
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29587
Patch to restore LTO bootstrap with Ada + comment tweaks

OK, this is the patch I've tested, but it's your call.

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

--- Comment #35 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-06 08:38:50 UTC ---
Author: rguenth
Date: Wed Mar  6 08:38:46 2013
New Revision: 196487

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196487
Log:
2013-03-06  Richard Biener  <rguenther@suse.de>

    PR middle-end/50494
    * tree-vect-data-refs.c (vect_can_force_dr_alignment_p):
    Do not adjust alignment of DECL_IN_CONSTANT_POOL decls.

    Revert
    2013-02-13  Richard Biener  <rguenther@suse.de>

    PR lto/50494
    * varasm.c (output_constant_def_1): Get the decl representing
    the constant as argument.
    (output_constant_def): Wrap output_constant_def_1.
    (make_decl_rtl): Use output_constant_def_1 with the decl
    representing the constant.
    (build_constant_desc): Optionally re-use a decl already
    representing the constant.
    (tree_output_constant_def): Adjust.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-vect-data-refs.c
    trunk/gcc/varasm.c

[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #36 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-06 08:45:22 UTC ---
Fixed.