[Bug tree-optimization/52571] New: vectorizer changes alignment of common symbols

[Bug tree-optimization/52571] New: vectorizer changes alignment of common symbols

[ian at airs dot com]

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

             Bug #: 52571
           Summary: vectorizer changes alignment of common symbols
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: ian@airs.com


Compile this file with current mainline with -O2 on an ELF system:

unsigned long int *p;

void
f (char **a)
{
  p = (unsigned long int *) a;
}

In the .s file I see this:

    .comm    p,8,8

Now compile it with -O3.  In the .s file I see this:

    .comm    p,8,16

The alignment of a common symbol should not change based on the compiler
optimization level.

This appears to happen in vect_compute_data_ref_alignment.

Perhaps vect_can_force_dr_alignment_p needs to check DECL_COMMON.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-03-12 22:10:16 UTC ---
Dup of either PR 48127 or PR 49379.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-03-13
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-03-13 08:34:51 UTC ---
Hmm, if that is actually the bug I wonder if we should recommend -fno-common
for performance reason :(

Why is the link editor not required to use the biggest alignment?  Or,
why should the alignment of a common symbol not change based on compiler
optimization level?  What's so special about commons here?  Thus, why
is this not a linker bug?

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[ian at airs dot com]

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

--- Comment #3 from Ian Lance Taylor <ian at airs dot com> 2012-03-13 15:48:36 UTC ---
I agree: if the symbol is always common, the linker should use the largest
alignment.  But the symbol need not always be common.  Consider one file with
"unsigned long int *p;" and another file with "unsigned long int *p = &i;". 
That is a valid use of common symbols.  But now gcc might mark the common
symbol as having an alignment of 16, while leaving the definition with an
alignment of 8.  The rules of common symbol linking mean that the definition
will override the common symbol.  But the definition might have an alignment of
8, and it might not be possible for the linker to fix that--and in any case,
the linker won't even try.

In other words, common symbols are special.  You can't assume that anything you
change about them will stick, because they might become, in effect, an
undefined symbol.  I suspect that vect_can_force_dr_alignment_p should be
checking DECL_COMMON for exactly the reasons that it checks DECL_EXTERNAL.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenther at suse dot de]

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

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> 2012-03-14 09:35:12 UTC ---
On Tue, 13 Mar 2012, ian at airs dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52571
> 
> --- Comment #3 from Ian Lance Taylor <ian at airs dot com> 2012-03-13 15:48:36 UTC ---
> I agree: if the symbol is always common, the linker should use the largest
> alignment.  But the symbol need not always be common.  Consider one file with
> "unsigned long int *p;" and another file with "unsigned long int *p = &i;". 
> That is a valid use of common symbols.  But now gcc might mark the common
> symbol as having an alignment of 16, while leaving the definition with an
> alignment of 8.  The rules of common symbol linking mean that the definition
> will override the common symbol.  But the definition might have an alignment of
> 8, and it might not be possible for the linker to fix that--and in any case,
> the linker won't even try.
> 
> In other words, common symbols are special.  You can't assume that anything you
> change about them will stick, because they might become, in effect, an
> undefined symbol.  I suspect that vect_can_force_dr_alignment_p should be
> checking DECL_COMMON for exactly the reasons that it checks DECL_EXTERNAL.

Ok, thanks for the clarification, I'll produce a patch.

Richard.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-03-14 13:01:01 UTC ---
Fixed.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-03-14 13:00:49 UTC ---
Author: rguenth
Date: Wed Mar 14 13:00:44 2012
New Revision: 185380

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

    PR tree-optimization/52571
    * tree-vect-data-refs.c (vect_compute_data_ref_alignment): Move
    flag_section_anchors check ...
    (vect_can_force_dr_alignment_p): ... here.  Do not re-align
    DECL_COMMON variables.

    * gcc.dg/vect/vect-2.c: Initialize arrays.
    * gcc.dg/vect/no-section-anchors-vect-34.c: Likewise.
    * gcc.target/i386/recip-vec-divf.c: Use -fno-common.
    * gcc.target/i386/recip-vec-sqrtf.c: Likewise.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/vect/no-section-anchors-vect-34.c
    trunk/gcc/testsuite/gcc.dg/vect/vect-2.c
    trunk/gcc/testsuite/gcc.target/i386/recip-vec-divf.c
    trunk/gcc/testsuite/gcc.target/i386/recip-vec-sqrtf.c
    trunk/gcc/tree-vect-data-refs.c

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-03-16 16:48:38 UTC ---
Author: rguenth
Date: Fri Mar 16 16:48:31 2012
New Revision: 185474

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

    PR tree-optimization/52603
    * tree-vect-data-refs.c (vect_can_force_dr_alignment_p): Clarify
    comment.

    Revert
    2012-03-14  Richard Guenther  <rguenther@suse.de>

    PR tree-optimization/52571
    * tree-vect-data-refs.c (vect_compute_data_ref_alignment): Move
    flag_section_anchors check ...
    (vect_can_force_dr_alignment_p): ... here.

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

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[ro at gcc dot gnu.org]

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

Rainer Orth <ro at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
                 CC|                            |ro at gcc dot gnu.org
         Resolution|FIXED                       |

--- Comment #8 from Rainer Orth <ro at gcc dot gnu.org> 2012-03-20 15:27:48 UTC ---
Unfortunately, the patch caused many testsuite failures on both
sparc-sun-solaris
and powerpc-apple-darwin, as can be seen e.g. at

  http://gcc.gnu.org/ml/gcc-testresults/2012-03/msg02102.html

Should I file a separate PR for this?

  Rainer

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[dominiq at lps dot ens.fr]

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

--- Comment #9 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2012-03-20 15:55:23 UTC ---
> Unfortunately, the patch caused many testsuite failures on both
> sparc-sun-solaris
> and powerpc-apple-darwin, as can be seen e.g. at
>
>   http://gcc.gnu.org/ml/gcc-testresults/2012-03/msg02102.html
>
> Should I file a separate PR for this?

PR52603. Could you test the patch?

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[dominiq at lps dot ens.fr]

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

--- Comment #10 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2012-03-20 15:56:55 UTC ---
> PR52603. Could you test the patch?

Sorry, pr52614.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[mrs at gcc dot gnu.org]

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

mrs@gcc.gnu.org <mrs at gcc dot gnu.org> changed:

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

--- Comment #11 from mrs at gcc dot gnu.org <mrs at gcc dot gnu.org> 2012-04-09 23:52:54 UTC ---
Ah, I had another thought.  COMDAT and LINKONCE things I don't think can be
realigned for all the same reasons that one cannot align COMMON.  I've not
thought about this long and hard, so, could be wrong, so, would be good to have
a C++ or a vectorizer person review the idea.  The idea is, if you compile one
translation unit with a vectorizor on, and another with it off, we wind up with
two instantiations, each with different alignment, and the one picked at the
end need not be either of them, but rather an explicit instantiation.   This
seems identical to what happens to common to me.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[rguenth at gcc dot gnu.org]

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

--- Comment #12 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-05-10 11:28:28 UTC ---
(In reply to comment #11)
> Ah, I had another thought.  COMDAT and LINKONCE things I don't think can be
> realigned for all the same reasons that one cannot align COMMON.  I've not
> thought about this long and hard, so, could be wrong, so, would be good to have
> a C++ or a vectorizer person review the idea.  The idea is, if you compile one
> translation unit with a vectorizor on, and another with it off, we wind up with
> two instantiations, each with different alignment, and the one picked at the
> end need not be either of them, but rather an explicit instantiation.   This
> seems identical to what happens to common to me.

The question is what the specification says the link editor needs to do
here.  IMHO it needs to pick the version with the biggest alignment
(what happens if you have two same comdat groups with different comdats
having their alignment bumped?)

This all asks for LTO to regularize symbols more aggressively, thus get
rid of DECL_COMMONs and of COMDAT/LINKONCE, too.

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-05-10 11:45:21 UTC ---
Most comdats (the possible exception of hidden ones or comdats defined in
non-pic code) are overridable symbols and thus not good candidates for
increasing alignment anyway (because they could be defined in some other shared
library or in the executable).

[Bug tree-optimization/52571] vectorizer changes alignment of common symbols

[mrs at gcc dot gnu.org]

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

--- Comment #14 from mrs at gcc dot gnu.org <mrs at gcc dot gnu.org> 2012-05-10 16:06:20 UTC ---
I'd disagree with that.  I suspect 99% of them are not overridden, just like
99% of C functions are not weak and not overridden either.  As for what should
be done, doesn't matter, as since the system places no constraint on selection
for the linker, there simply is nothing one can depend upon.  I agree,
selecting the one with the maximal alignment would be nice, but, just be wary
this may cause one to try and make use of that property, and this just would
turn into a complex form of bug pushing or bug hiding.

Uncommoning things I think might well be slightly harder than trivial.  A ctor
is allowed to dynamically load a shared library, that defines a symbol, maybe,
conditional on the environment.  But with proper escape analysis, seems like
one can figure it all out, just be careful with the edge cases.