Patchwork Fix PR52614

login
register
mail settings
Submitter William J. Schmidt
Date April 5, 2012, 2:56 a.m.
Message ID <1333594591.19102.40.camel@gnopaine>
Download mbox | patch
Permalink /patch/150843/
State New
Headers show

Comments

William J. Schmidt - April 5, 2012, 2:56 a.m.
There seems to be tacit agreement that the vector tests should use
-fno-common on all targets to avoid the recent spate of failures (see
discussion in 52571 and 52603).  This patch (proposed by Dominique
D'Humieures) does just that.  I agreed to shepherd the patch through.
I've verified that it removes the failures for powerpc64-linux.  Various
others have verified for arm, sparc, and darwin.  OK for trunk?

Thanks,
Bill


gcc/testsuite:

2012-04-04  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
            Dominique D'Humieures <dominiq@lps.ens.fr>

	PR testsuite/52614
	* gcc.dg/vect/vect.exp: Use -fno-common on all targets.
	* gcc.dg/vect/costmodel/ppc/ppc-costmodel-vect.exp: Likewise.
Mike Stump - April 5, 2012, 4:22 a.m.
On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
> There seems to be tacit agreement that the vector tests should use
> -fno-common on all targets to avoid the recent spate of failures (see
> discussion in 52571 and 52603).

> OK for trunk?

Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.
Richard Guenther - April 5, 2012, 9:30 a.m.
On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
>> There seems to be tacit agreement that the vector tests should use
>> -fno-common on all targets to avoid the recent spate of failures (see
>> discussion in 52571 and 52603).
>
>> OK for trunk?
>
> Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.

Ian, you are the "source" of all of these problems.  While I did not notice
any degradations in SPEC (on x86) with handling commons "correctly"
now, the fact
that our testsuite needs -fno-common to make things vectorizable shows
that users might be impacted negatively by this, which is only a real problem
in corner cases.  Why can the link editor not promote the definitions alignment
when merging with a common with bigger alignment?

Richard.
William J. Schmidt - April 5, 2012, 11:59 a.m.
On Thu, 2012-04-05 at 11:30 +0200, Richard Guenther wrote:
> On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump <mikestump@comcast.net> wrote:
> > On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
> >> There seems to be tacit agreement that the vector tests should use
> >> -fno-common on all targets to avoid the recent spate of failures (see
> >> discussion in 52571 and 52603).
> >
> >> OK for trunk?
> >
> > Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.
> 
> Ian, you are the "source" of all of these problems.  While I did not notice
> any degradations in SPEC (on x86) with handling commons "correctly"
> now, the fact
> that our testsuite needs -fno-common to make things vectorizable shows
> that users might be impacted negatively by this, which is only a real problem
> in corner cases.  Why can the link editor not promote the definitions alignment
> when merging with a common with bigger alignment?
> 
> Richard.
> 

Follow-up question:  Should -ftree-vectorize imply -fno-common in the
short term?

Thanks,
Bill
Richard Guenther - April 5, 2012, 12:26 p.m.
On Thu, Apr 5, 2012 at 1:59 PM, William J. Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> On Thu, 2012-04-05 at 11:30 +0200, Richard Guenther wrote:
>> On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump <mikestump@comcast.net> wrote:
>> > On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
>> >> There seems to be tacit agreement that the vector tests should use
>> >> -fno-common on all targets to avoid the recent spate of failures (see
>> >> discussion in 52571 and 52603).
>> >
>> >> OK for trunk?
>> >
>> > Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.
>>
>> Ian, you are the "source" of all of these problems.  While I did not notice
>> any degradations in SPEC (on x86) with handling commons "correctly"
>> now, the fact
>> that our testsuite needs -fno-common to make things vectorizable shows
>> that users might be impacted negatively by this, which is only a real problem
>> in corner cases.  Why can the link editor not promote the definitions alignment
>> when merging with a common with bigger alignment?
>>
>> Richard.
>>
>
> Follow-up question:  Should -ftree-vectorize imply -fno-common in the
> short term?

That's probably more a C language question - you would get valid C
rejected with -fno-common.  But maybe -ftree-vectorize should suggest
-fno-common when it encounters a case it would like to promote alignment
for.  Not sure if for example Fortran would ever work with -fno-common though.

Richard.

> Thanks,
> Bill
>
Mike Stump - April 5, 2012, 5:31 p.m.
On Apr 5, 2012, at 2:30 AM, Richard Guenther wrote:
> Why can the link editor not promote the definitions alignment
> when merging with a common with bigger alignment?

The problem is that when a common symbol is upped in alignment, but then not chosen by ld (or worse, by the dynamic linker), but the codegen that assumes a larger alignment is used, then you realize the alignment of the data actually selected must be retroactively upped as well.  This means, the alignment in existing .a files, .o files, and .so files.  The .a and .o files can be easily solved by simply requiring the compile of the world after upgrading all linkers (static and dynamic) to only have -fdata-sections.  For .so files, well, that's above my pay grade.  Darwin, I'll note, can manage the upping on the size and alignment, but only between all the commons, not a hard definition behind it.
Ian Taylor - April 5, 2012, 10:15 p.m.
Richard Guenther <richard.guenther@gmail.com> writes:

> On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump <mikestump@comcast.net> wrote:
>> On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
>>> There seems to be tacit agreement that the vector tests should use
>>> -fno-common on all targets to avoid the recent spate of failures (see
>>> discussion in 52571 and 52603).
>>
>>> OK for trunk?
>>
>> Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.
>
> Ian, you are the "source" of all of these problems.  While I did not notice
> any degradations in SPEC (on x86) with handling commons "correctly"
> now, the fact
> that our testsuite needs -fno-common to make things vectorizable shows
> that users might be impacted negatively by this, which is only a real problem
> in corner cases.  Why can the link editor not promote the definitions alignment
> when merging with a common with bigger alignment?

The defined symbol will be embedded in a data section along with other
defined data symbols, at some offset O from the start of the section.
The data section will have a required alignment A.  It is entirely
possible that there is no way to honor A and also ensure that A+O is
aligned as requested by the common symbol.

This whole issue only applies to vectorization involving global symbols
defined with no initializer in C, when vectorization requires an
alignment greater than that implied by the type of the symbol.  It does
not affect function arguments or local variables.  It does not affect
C++.  Is vectorization of uninitialized global symbols really a common
case?

Another approach we could use is to say that when the vectorizer wants
to increase the alignment of a common symbol, it would force the symbol
to be defined rather than common.  That would be safe, as it would lead
to a multiple-definition error at link time in cases where it would be
unsafe.  I would be fine with making the compiler work that way for C,
although of course there would have to be an option to disable it.

I don't know enough about Fortran to know whether the same issues arise
there.  Perhaps in Fortran a common symbol is always a common symbol and
can never be a defined symbol.  If that is the case then for Fortran I
think it would be safe to change the alignment of the common symbol.  Of
course we would need to have some way to indicate that in the
middle-end--DECL_ALWAYS_COMMON or something.

Ian
Richard Guenther - April 9, 2012, 10:34 a.m.
On Fri, Apr 6, 2012 at 12:15 AM, Ian Lance Taylor <iant@google.com> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>
>> On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Apr 4, 2012, at 7:56 PM, William J. Schmidt wrote:
>>>> There seems to be tacit agreement that the vector tests should use
>>>> -fno-common on all targets to avoid the recent spate of failures (see
>>>> discussion in 52571 and 52603).
>>>
>>>> OK for trunk?
>>>
>>> Ok.  Any other solution I think will be real work and we shouldn't loose the testing between now and then by not having the test cases working.
>>
>> Ian, you are the "source" of all of these problems.  While I did not notice
>> any degradations in SPEC (on x86) with handling commons "correctly"
>> now, the fact
>> that our testsuite needs -fno-common to make things vectorizable shows
>> that users might be impacted negatively by this, which is only a real problem
>> in corner cases.  Why can the link editor not promote the definitions alignment
>> when merging with a common with bigger alignment?
>
> The defined symbol will be embedded in a data section along with other
> defined data symbols, at some offset O from the start of the section.
> The data section will have a required alignment A.  It is entirely
> possible that there is no way to honor A and also ensure that A+O is
> aligned as requested by the common symbol.
>
> This whole issue only applies to vectorization involving global symbols
> defined with no initializer in C, when vectorization requires an
> alignment greater than that implied by the type of the symbol.  It does
> not affect function arguments or local variables.  It does not affect
> C++.  Is vectorization of uninitialized global symbols really a common
> case?
>
> Another approach we could use is to say that when the vectorizer wants
> to increase the alignment of a common symbol, it would force the symbol
> to be defined rather than common.  That would be safe, as it would lead
> to a multiple-definition error at link time in cases where it would be
> unsafe.  I would be fine with making the compiler work that way for C,
> although of course there would have to be an option to disable it.

That would work fine at least with LTO - though I'm not sure the linker-plugin
gives enough feedback that a common is not overridden by an external
definition.  Maybe it would be a good idea to promote all commons to
definitions with LTO (where possible, of course).

> I don't know enough about Fortran to know whether the same issues arise
> there.  Perhaps in Fortran a common symbol is always a common symbol and
> can never be a defined symbol.  If that is the case then for Fortran I
> think it would be safe to change the alignment of the common symbol.  Of
> course we would need to have some way to indicate that in the
> middle-end--DECL_ALWAYS_COMMON or something.

I don't know either.  OTOH commons are records, so the vectorizer cannot
change alignment of a common sub-variable - so in this case it would be
better if the frontend would align the common to BIGGEST_ALIGNMENT or so.

Richard.

> Ian

Patch

Index: gcc/testsuite/gcc.dg/vect/costmodel/ppc/ppc-costmodel-vect.exp
===================================================================
--- gcc/testsuite/gcc.dg/vect/costmodel/ppc/ppc-costmodel-vect.exp	(revision 186108)
+++ gcc/testsuite/gcc.dg/vect/costmodel/ppc/ppc-costmodel-vect.exp	(working copy)
@@ -34,7 +34,7 @@  if ![is-effective-target powerpc_altivec_ok] {
 set DEFAULT_VECTCFLAGS ""
 
 # These flags are used for all targets.
-lappend DEFAULT_VECTCFLAGS "-O2" "-ftree-vectorize" "-fvect-cost-model"
+lappend DEFAULT_VECTCFLAGS "-O2" "-ftree-vectorize" "-fvect-cost-model" "-fno-common"
 
 # If the target system supports vector instructions, the default action
 # for a test is 'run', otherwise it's 'compile'.  Save current default.
Index: gcc/testsuite/gcc.dg/vect/vect.exp
===================================================================
--- gcc/testsuite/gcc.dg/vect/vect.exp	(revision 186108)
+++ gcc/testsuite/gcc.dg/vect/vect.exp	(working copy)
@@ -40,7 +40,7 @@  if ![check_vect_support_and_set_flags] {
 }
 
 # These flags are used for all targets.
-lappend DEFAULT_VECTCFLAGS "-ftree-vectorize" "-fno-vect-cost-model"
+lappend DEFAULT_VECTCFLAGS "-ftree-vectorize" "-fno-vect-cost-model" "-fno-common"
 
 # Initialize `dg'.
 dg-init