| Message ID | 1456423200-16337-1-git-send-email-alan.lawrence@arm.com |
|---|---|
| State | New |
| Headers | show |
On 25/02/16 18:00, Alan Lawrence wrote: > On 22/02/16 12:03, Jakub Jelinek wrote: >> (f) A global command-line option, which we check alongside DECL_COMMON and >> further tests (basically, we want only DECL_COMMON decls that either have >> ARRAY_TYPE, or some other aggregate type with flexible array member or some >> other trailing array in the struct), and use the resulting predicate in >> places where we optimize based on DECL_SIZE{,_UNIT} of the decl - if that >> predicate returns true, we assume the DECL_SIZE is >> "variable"/"unlimited"/whatever. >> The two spots known to me that would need to use this new predicate would >> be: >> tree.c (array_at_struct_end_p): > [...] >> tree-dfa.c (get_ref_base_and_extent): > [...] > > Close enough to what I meant by (a)/(b), but never mind that, and I hadn't > looked at where the change for aggressive loop opts needed to be. However... > Well you are essentially writing the patch for me at this point (so, thanks!), > but here's a patch that puts that under a global -funknown-commons flag. > Testcase as before. > > Bootstrapped (with flag overridden to true) on x86_64 and aarch64, check-gcc, > check-fortran, and tested 416.gamess on AArch64, where this patch enables > running *without* the -fno-aggressive-loop-opts previously required. > > In the gcc testsuite, these fail with the option turned on: > gcc.dg/pr53265.c (test for warnings, line 137) > gcc.dg/pr53265.c (test for warnings, line 138) > gcc.dg/pr64277.c scan-tree-dump cunroll ... (*2) > gcc.dg/tree-ssa/cunroll-{1,2,3,4,9,10,11}.c scan-tree-dump cunroll/cunrolli/ivcanon (various) > gcc.dg/tree-ssa/loop-38.c scan-tree-dump cunrolli "Loop 1 iterates at most 11 times" > ...which all appear harmless. > > Thomas Koenig suggests (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69368#c80) > that this be combined with some flag fiddling and warnings in the Fortran > front-end; this patch doesn't do that, as I'm not very familiar with the > frontends, but that can follow in a separate patch. (Thomas?) > > OK for trunk? > > Cheers, Alan > > gcc/ChangeLog: > > DATE Alan Lawrence <alan.lawrence@arm.com> > Jakub Jelinek <jakub@redhat.com> > > * common.opt (funknown-commons, flag_unknown_commons): New. > * tree.c (array_at_struct_end_p): Do not limit to size of decl for > DECL_COMMONS if flag_unknown_commons is set. > * tree-dfa.c (get_ref_base_and_extent): Likewise. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/unknown_commons.f: New. Ping. (Besides my original patch https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01356.html which we decided was too fragile, I believe this is the only patch proposed that actually fixes the miscompare in 416.gamess.) Thanks, Alan
On Thu, Feb 25, 2016 at 7:00 PM, Alan Lawrence <alan.lawrence@arm.com> wrote: > On 22/02/16 12:03, Jakub Jelinek wrote: >> (f) A global command-line option, which we check alongside DECL_COMMON and >> further tests (basically, we want only DECL_COMMON decls that either have >> ARRAY_TYPE, or some other aggregate type with flexible array member or some >> other trailing array in the struct), and use the resulting predicate in >> places where we optimize based on DECL_SIZE{,_UNIT} of the decl - if that >> predicate returns true, we assume the DECL_SIZE is >> "variable"/"unlimited"/whatever. >> The two spots known to me that would need to use this new predicate would >> be: >> tree.c (array_at_struct_end_p): > [...] >> tree-dfa.c (get_ref_base_and_extent): > [...] > > Close enough to what I meant by (a)/(b), but never mind that, and I hadn't > looked at where the change for aggressive loop opts needed to be. However... > Well you are essentially writing the patch for me at this point (so, thanks!), > but here's a patch that puts that under a global -funknown-commons flag. > Testcase as before. > > Bootstrapped (with flag overridden to true) on x86_64 and aarch64, check-gcc, > check-fortran, and tested 416.gamess on AArch64, where this patch enables > running *without* the -fno-aggressive-loop-opts previously required. > > In the gcc testsuite, these fail with the option turned on: > gcc.dg/pr53265.c (test for warnings, line 137) > gcc.dg/pr53265.c (test for warnings, line 138) > gcc.dg/pr64277.c scan-tree-dump cunroll ... (*2) > gcc.dg/tree-ssa/cunroll-{1,2,3,4,9,10,11}.c scan-tree-dump cunroll/cunrolli/ivcanon (various) > gcc.dg/tree-ssa/loop-38.c scan-tree-dump cunrolli "Loop 1 iterates at most 11 times" > ...which all appear harmless. > > Thomas Koenig suggests (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69368#c80) > that this be combined with some flag fiddling and warnings in the Fortran > front-end; this patch doesn't do that, as I'm not very familiar with the > frontends, but that can follow in a separate patch. (Thomas?) > > OK for trunk? Comments below. > Cheers, Alan > > gcc/ChangeLog: > > DATE Alan Lawrence <alan.lawrence@arm.com> > Jakub Jelinek <jakub@redhat.com> > > * common.opt (funknown-commons, flag_unknown_commons): New. > * tree.c (array_at_struct_end_p): Do not limit to size of decl for > DECL_COMMONS if flag_unknown_commons is set. > * tree-dfa.c (get_ref_base_and_extent): Likewise. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/unknown_commons.f: New. > --- > gcc/common.opt | 4 ++++ > gcc/testsuite/gfortran.dg/unknown_commons.f | 20 ++++++++++++++++++++ > gcc/tree-dfa.c | 15 ++++++++++++++- > gcc/tree.c | 6 ++++-- > 4 files changed, 42 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gfortran.dg/unknown_commons.f > > diff --git a/gcc/common.opt b/gcc/common.opt > index 520fa9c..b5b1282 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -2455,6 +2455,10 @@ funit-at-a-time > Common Report Var(flag_unit_at_a_time) Init(1) > Compile whole compilation unit at a time. > > +funknown-commons > +Common Var(flag_unknown_commons) > +Assume common declarations may be overridden with unknown larger sizes I think to make it work with LTO you need to mark it 'Optimization'. Also it's about arrays so maybe 'Assume common declarations may be overridden with ones with a larger trailing array' also if we document it here we should eventually document it in invoke.texi. Not sure if "unknown commons" is a good term, maybe "unconstrained commons" instead? Otherwise looks ok to me. Richard. > + > funroll-loops > Common Report Var(flag_unroll_loops) Optimization > Perform loop unrolling when iteration count is known. > diff --git a/gcc/testsuite/gfortran.dg/unknown_commons.f b/gcc/testsuite/gfortran.dg/unknown_commons.f > new file mode 100644 > index 0000000..97e3ce3 > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/unknown_commons.f > @@ -0,0 +1,20 @@ > +! { dg-do compile } > +! { dg-options "-O3 -funknown-commons -fdump-tree-dom2-details" } > + > +! Test for PR69368: a single-element array in a common block, which will be > +! overridden with a larger size at link time (contrary to language spec). > +! Dominator opts considers accesses to differently-computed elements of X as > +! equivalent, unless -funknown-commons is passed in. > + SUBROUTINE FOO > + IMPLICIT DOUBLE PRECISION (X) > + INTEGER J > + COMMON /MYCOMMON / X(1) > + DO 10 J=1,1024 > + X(J+1)=X(J+7) > + 10 CONTINUE > + RETURN > + END > +! { dg-final { scan-tree-dump-not "FIND" "dom2" } } > +! We should retain both a read and write of mycommon.x. > +! { dg-final { scan-tree-dump-times " _\[0-9\]+ = mycommon\\.x\\\[_\[0-9\]+\\\];" 1 "dom2" } } > +! { dg-final { scan-tree-dump-times " mycommon\\.x\\\[_\[0-9\]+\\\] = _\[0-9\]+;" 1 "dom2" } } > diff --git a/gcc/tree-dfa.c b/gcc/tree-dfa.c > index 0e98056..017e98e 100644 > --- a/gcc/tree-dfa.c > +++ b/gcc/tree-dfa.c > @@ -612,9 +612,22 @@ get_ref_base_and_extent (tree exp, HOST_WIDE_INT *poffset, > > if (DECL_P (exp)) > { > + if (flag_unknown_commons > + && TREE_CODE (exp) == VAR_DECL && DECL_COMMON (exp)) > + { > + tree sz_tree = TYPE_SIZE (TREE_TYPE (exp)); > + /* If size is unknown, or we have read to the end, assume there > + may be more to the structure than we are told. */ > + if (TREE_CODE (TREE_TYPE (exp)) == ARRAY_TYPE > + || (seen_variable_array_ref > + && (sz_tree == NULL_TREE > + || TREE_CODE (sz_tree) != INTEGER_CST > + || (bit_offset + maxsize == wi::to_offset (sz_tree))))) > /* If maxsize is unknown adjust it according to the size of the > base decl. */ > - if (maxsize == -1 > + else if (maxsize == -1 > && DECL_SIZE (exp) > && TREE_CODE (DECL_SIZE (exp)) == INTEGER_CST) > maxsize = wi::to_offset (DECL_SIZE (exp)) - bit_offset; > diff --git a/gcc/tree.c b/gcc/tree.c > index 07cb9d9..eb6b642 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -12957,8 +12957,10 @@ array_at_struct_end_p (tree ref) > } > > /* If the reference is based on a declared entity, the size of the array > - is constrained by its given domain. */ > - if (DECL_P (ref)) > + is constrained by its given domain. (Do not trust commons PR/69368). */ > + if (DECL_P (ref) > + && !(flag_unknown_commons > + && TREE_CODE (ref) == VAR_DECL && DECL_COMMON (ref))) > return false; > > return true; > -- > 1.9.1 >
On Fri, Mar 04, 2016 at 02:27:46PM +0100, Richard Biener wrote: > > +funknown-commons > > +Common Var(flag_unknown_commons) > > +Assume common declarations may be overridden with unknown larger sizes > > I think to make it work with LTO you need to mark it 'Optimization'. > Also it's about > arrays so maybe > > 'Assume common declarations may be overridden with ones with a larger > trailing array' > > also if we document it here we should eventually document it in invoke.texi. Also, isn't the *.opt description line supposed to end with a full stop? Jakub
diff --git a/gcc/common.opt b/gcc/common.opt index 520fa9c..b5b1282 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -2455,6 +2455,10 @@ funit-at-a-time Common Report Var(flag_unit_at_a_time) Init(1) Compile whole compilation unit at a time. +funknown-commons +Common Var(flag_unknown_commons) +Assume common declarations may be overridden with unknown larger sizes + funroll-loops Common Report Var(flag_unroll_loops) Optimization Perform loop unrolling when iteration count is known. diff --git a/gcc/testsuite/gfortran.dg/unknown_commons.f b/gcc/testsuite/gfortran.dg/unknown_commons.f new file mode 100644 index 0000000..97e3ce3 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/unknown_commons.f @@ -0,0 +1,20 @@ +! { dg-do compile } +! { dg-options "-O3 -funknown-commons -fdump-tree-dom2-details" } + +! Test for PR69368: a single-element array in a common block, which will be +! overridden with a larger size at link time (contrary to language spec). +! Dominator opts considers accesses to differently-computed elements of X as +! equivalent, unless -funknown-commons is passed in. + SUBROUTINE FOO + IMPLICIT DOUBLE PRECISION (X) + INTEGER J + COMMON /MYCOMMON / X(1) + DO 10 J=1,1024 + X(J+1)=X(J+7) + 10 CONTINUE + RETURN + END +! { dg-final { scan-tree-dump-not "FIND" "dom2" } } +! We should retain both a read and write of mycommon.x. +! { dg-final { scan-tree-dump-times " _\[0-9\]+ = mycommon\\.x\\\[_\[0-9\]+\\\];" 1 "dom2" } } +! { dg-final { scan-tree-dump-times " mycommon\\.x\\\[_\[0-9\]+\\\] = _\[0-9\]+;" 1 "dom2" } } diff --git a/gcc/tree-dfa.c b/gcc/tree-dfa.c index 0e98056..017e98e 100644 --- a/gcc/tree-dfa.c +++ b/gcc/tree-dfa.c @@ -612,9 +612,22 @@ get_ref_base_and_extent (tree exp, HOST_WIDE_INT *poffset, if (DECL_P (exp)) { + if (flag_unknown_commons + && TREE_CODE (exp) == VAR_DECL && DECL_COMMON (exp)) + { + tree sz_tree = TYPE_SIZE (TREE_TYPE (exp)); + /* If size is unknown, or we have read to the end, assume there + may be more to the structure than we are told. */ + if (TREE_CODE (TREE_TYPE (exp)) == ARRAY_TYPE + || (seen_variable_array_ref + && (sz_tree == NULL_TREE + || TREE_CODE (sz_tree) != INTEGER_CST + || (bit_offset + maxsize == wi::to_offset (sz_tree))))) + maxsize = -1; + } /* If maxsize is unknown adjust it according to the size of the base decl. */ - if (maxsize == -1 + else if (maxsize == -1 && DECL_SIZE (exp) && TREE_CODE (DECL_SIZE (exp)) == INTEGER_CST) maxsize = wi::to_offset (DECL_SIZE (exp)) - bit_offset; diff --git a/gcc/tree.c b/gcc/tree.c index 07cb9d9..eb6b642 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -12957,8 +12957,10 @@ array_at_struct_end_p (tree ref) } /* If the reference is based on a declared entity, the size of the array - is constrained by its given domain. */ - if (DECL_P (ref)) + is constrained by its given domain. (Do not trust commons PR/69368). */ + if (DECL_P (ref) + && !(flag_unknown_commons + && TREE_CODE (ref) == VAR_DECL && DECL_COMMON (ref))) return false; return true;