This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add -funknown-commons to work around PR/69368 (and others) in SPEC2006
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Alan Lawrence <alan dot lawrence at arm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>, Thomas Koenig <tkoenig at gcc dot gnu dot org>
- Date: Fri, 4 Mar 2016 14:27:46 +0100
- Subject: Re: [PATCH] Add -funknown-commons to work around PR/69368 (and others) in SPEC2006
- Authentication-results: sourceware.org; auth=none
- References: <20160222120328 dot GH3017 at tucnak dot redhat dot com> <1456423200-16337-1-git-send-email-alan dot lawrence at arm dot com>
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
>