Re: [PATCH] Fix PR52614
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 if this answers the question, but I have run the gfortran test suite with -fno-common and I have seen only one failure: [macbook] lin/test% gfc /opt/gcc/work/gcc/testsuite/gfortran.dg/global_vars_f90_init.f90 /opt/gcc/work/gcc/testsuite/gfortran.dg/global_vars_f90_init_driver.c -fno-common -save-temps ld: duplicate symbol _i in global_vars_f90_init_driver.o and global_vars_f90_init.o collect2: error: ld returned 1 exit status AFAICT, COMMON is only lightly tested in the gfortran test suite and mostly for errors or warnings. However I find surprising that gfortran.dg/bind_c_coms.f90 + gfortran.dg/bind_c_coms_driver.c works: [macbook] lin/test% gfc /opt/gcc/work/gcc/testsuite/gfortran.dg/bind_c_coms.f90 /opt/gcc/work/gcc/testsuite/gfortran.dg/bind_c_coms_driver.c -fno-common [macbook] lin/test% a.out [macbook] lin/test% Dominique
Re: [PATCH] Fix PR52614
On Tue, Apr 10, 2012 at 1:00 PM, Dominique Dhumieres domi...@lps.ens.fr wrote: 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 if this answers the question, but I have run the gfortran test suite with -fno-common and I have seen only one failure: [macbook] lin/test% gfc /opt/gcc/work/gcc/testsuite/gfortran.dg/global_vars_f90_init.f90 /opt/gcc/work/gcc/testsuite/gfortran.dg/global_vars_f90_init_driver.c -fno-common -save-temps ld: duplicate symbol _i in global_vars_f90_init_driver.o and global_vars_f90_init.o collect2: error: ld returned 1 exit status AFAICT, COMMON is only lightly tested in the gfortran test suite and mostly for errors or warnings. However I find surprising that gfortran.dg/bind_c_coms.f90 + gfortran.dg/bind_c_coms_driver.c works: -fno-common seems to be ignored by the fortran frontend, thus, subroutine foo common/BAR/ x real(8) x end BAR is still a common decl with -fno-common. For your testcase only the C compiled parts end up not using commons. Thus the vectorizer would fail to promote the alignment of BAR regardless of -f[no-]common. GFortran OTOH does .comm bar_,8,16 for the above, thus aligns it to 16 already (to 32 with -mavx, probably for vectorization). So we should be fine. Richard. [macbook] lin/test% gfc /opt/gcc/work/gcc/testsuite/gfortran.dg/bind_c_coms.f90 /opt/gcc/work/gcc/testsuite/gfortran.dg/bind_c_coms_driver.c -fno-common [macbook] lin/test% a.out [macbook] lin/test% Dominique
Re: [PATCH] Fix PR52614
On Fri, Apr 6, 2012 at 12:15 AM, Ian Lance Taylor i...@google.com wrote: Richard Guenther richard.guent...@gmail.com writes: On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump mikest...@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
Re: [PATCH] Fix PR52614
On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump mikest...@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.
Re: [PATCH] Fix PR52614
On Thu, 2012-04-05 at 11:30 +0200, Richard Guenther wrote: On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump mikest...@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
Re: [PATCH] Fix PR52614
On Thu, Apr 5, 2012 at 1:59 PM, William J. Schmidt wschm...@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 mikest...@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
Re: [PATCH] Fix PR52614
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.
Re: [PATCH] Fix PR52614
Richard Guenther richard.guent...@gmail.com writes: On Thu, Apr 5, 2012 at 6:22 AM, Mike Stump mikest...@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
[PATCH] Fix PR52614
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 wschm...@linux.vnet.ibm.com
Dominique D'Humieures domi...@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.
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
Re: [PATCH] Fix PR52614
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.
