| Message ID | 1455903754-12572-1-git-send-email-alan.lawrence@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 19, 2016 at 05:42:34PM +0000, Alan Lawrence wrote: > This relates to FORTRAN code where different modules give different sizes to the > same array in a COMMON block (contrary to the fortran language specification). > SPEC have refused to patch the source code > (https://www.spec.org/cpu2006/Docs/faq.html#Run.05). > > Hence, this patch provides a Fortran-specific option -funknown-commons that > marks such arrays as having unknown size - that is, NULL_TREE for both > TYPE_SIZE and max value of TYPE_DOMAIN. DECL_SIZE is preserved for e.g. output > in varasm.c. > > On AArch64, it fixes the 416.gamess issue, and allows compiling 416.gamess > without the -fno-aggressive-loop-optimizations previously required (numerous > other PRs relating to 416.gamess). > > I had to fix up a couple of places to deal with null TYPE_SIZE but in most cases I think it is wrong to touch TYPE_SIZE/TYPE_SIZE_UNIT, IMHO it is much better just to ignore DECL_SIZE/DECL_SIZE_UNIT in the selected few places (get_ref_base_and_extent, the tree-ssa-loop-niters.c analysis) if the switch is on, for selected decls (aggregates with flexible array members and other similar trailing arrays, arrays themselves; all only if DECL_COMMON). > a test for such was already present. (omp-low.c had too many so I gave up: in > practice I think it's OK to just not use the new flag at the same time as > -fopenmp). That will just be an endless source of bugreports when people will report ICEs with -funknown-commons -fopenmp (or -fopenacc, or -fcilkplus, or -ftree-parallelize-loops, ...). For OpenMP the TYPE_SIZE* is essential, if you privatize variables, you need to know how big variable to allocate etc. Jakub
On 19/02/16 17:52, Jakub Jelinek wrote: > On Fri, Feb 19, 2016 at 05:42:34PM +0000, Alan Lawrence wrote: >> This relates to FORTRAN code where different modules give different sizes to the >> same array in a COMMON block (contrary to the fortran language specification). >> SPEC have refused to patch the source code >> (https://www.spec.org/cpu2006/Docs/faq.html#Run.05). >> >> Hence, this patch provides a Fortran-specific option -funknown-commons that >> marks such arrays as having unknown size - that is, NULL_TREE for both >> TYPE_SIZE and max value of TYPE_DOMAIN. DECL_SIZE is preserved for e.g. output >> in varasm.c. >> >> On AArch64, it fixes the 416.gamess issue, and allows compiling 416.gamess >> without the -fno-aggressive-loop-optimizations previously required (numerous >> other PRs relating to 416.gamess). >> >> I had to fix up a couple of places to deal with null TYPE_SIZE but in most cases > > I think it is wrong to touch TYPE_SIZE/TYPE_SIZE_UNIT, IMHO it is much better just > to ignore DECL_SIZE/DECL_SIZE_UNIT in the selected few places > (get_ref_base_and_extent, the tree-ssa-loop-niters.c analysis) if the switch > is on, for selected decls (aggregates with flexible array members and other > similar trailing arrays, arrays themselves; all only if DECL_COMMON). So do you see... (a) A global command-line option, which we check alongside DECL_COMMON, in (all or some of the) places which currently deal with flexible array members? (I guess the flag would have no effect for compiling C code...or (b) do we require it to 'enable' the existing flexible array member support?) (c) A new flag on each DECL, which we check in the places dealing with flexible array members, which we set on those decls in the fortran frontend if a fortran-frontend-only command-line option is passed? (d) A new flag on each DECL, which replaces the check for flexible array members, plus a global command-line option, which controls both setting the flag (on DECL_COMMONs) in the fortran frontend, and also setting it on flexible array members in the C frontend? This might be 'cleanest' but is also probably the most change and I doubt I'm going to have time to do this for GCC 6... (e) Some other scheme that I've not understood yet? Thanks, Alan
On Mon, Feb 22, 2016 at 11:46:19AM +0000, Alan Lawrence wrote: > On 19/02/16 17:52, Jakub Jelinek wrote: > >On Fri, Feb 19, 2016 at 05:42:34PM +0000, Alan Lawrence wrote: > >>This relates to FORTRAN code where different modules give different sizes to the > >>same array in a COMMON block (contrary to the fortran language specification). > >>SPEC have refused to patch the source code > >>(https://www.spec.org/cpu2006/Docs/faq.html#Run.05). > >> > >>Hence, this patch provides a Fortran-specific option -funknown-commons that > >>marks such arrays as having unknown size - that is, NULL_TREE for both > >>TYPE_SIZE and max value of TYPE_DOMAIN. DECL_SIZE is preserved for e.g. output > >>in varasm.c. > >> > >>On AArch64, it fixes the 416.gamess issue, and allows compiling 416.gamess > >>without the -fno-aggressive-loop-optimizations previously required (numerous > >>other PRs relating to 416.gamess). > >> > >>I had to fix up a couple of places to deal with null TYPE_SIZE but in most cases > > > >I think it is wrong to touch TYPE_SIZE/TYPE_SIZE_UNIT, IMHO it is much better just > >to ignore DECL_SIZE/DECL_SIZE_UNIT in the selected few places > >(get_ref_base_and_extent, the tree-ssa-loop-niters.c analysis) if the switch > >is on, for selected decls (aggregates with flexible array members and other > >similar trailing arrays, arrays themselves; all only if DECL_COMMON). > > So do you see... > > (a) A global command-line option, which we check alongside DECL_COMMON, in > (all or some of the) places which currently deal with flexible array > members? (I guess the flag would have no effect for compiling C code...or > (b) do we require it to 'enable' the existing flexible array member support?) > > (c) A new flag on each DECL, which we check in the places dealing with > flexible array members, which we set on those decls in the fortran frontend > if a fortran-frontend-only command-line option is passed? > > (d) A new flag on each DECL, which replaces the check for flexible array > members, plus a global command-line option, which controls both setting the > flag (on DECL_COMMONs) in the fortran frontend, and also setting it on > flexible array members in the C frontend? This might be 'cleanest' but is > also probably the most change and I doubt I'm going to have time to do this > for GCC 6... > > (e) Some other scheme that I've not understood yet? (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): /* If the reference is based on a declared entity, the size of the array is constrained by its given domain. */ if (DECL_P (ref)) return false; This would need to do if (DECL_P (ref) && !the_new_predicate (ref)) return false; tree-dfa.c (get_ref_base_and_extent): if (DECL_P (exp)) { ... } You want to do inside of that block something like if (the_new_predicate (exp)) { /* We need to deal with variable arrays ending structures. */ if (seen_variable_array_ref && maxsize != -1 && (TYPE_SIZE (TREE_TYPE (exp)) == NULL_TREE || TREE_CODE (TYPE_SIZE (TREE_TYPE (exp))) != INTEGER_CST || (bit_offset + maxsize == wi::to_offset (TYPE_SIZE (TREE_TYPE (exp)))))) maxsize = -1; else if (TREE_CODE (TREE_TYPE (exp)) == ARRAY_TYPE) maxsize = -1; } /* If maxsize is unknown adjust it according to the size of the base decl. */ else if ((maxsize == -1 && DECL_SIZE (exp) && TREE_CODE (DECL_SIZE (exp)) == INTEGER_CST) maxsize = wi::to_offset (DECL_SIZE (exp)) - bit_offset; Thus, basically for the DECL_COMMON with trailing array readd the r233153 hunk (though it can be readded in slightly different spot), somehow deal with the case of ARRAY_TYPE itself, and only apply DEC_SIZE if the predicate is false. Jakub
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): Well, with just those two, this fixes 416.gamess, and passes all tests in gfortran, with only a few scan-dump/warning tests failing in gcc... --Alan
diff --git a/gcc/expr.c b/gcc/expr.c index f4bac36..609bf59 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -6638,6 +6638,7 @@ store_field (rtx target, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos, RHS isn't the same size as the bitfield, we must use bitfield operations. */ || (bitsize >= 0 + && TYPE_SIZE (TREE_TYPE (exp)) && TREE_CODE (TYPE_SIZE (TREE_TYPE (exp))) == INTEGER_CST && compare_tree_int (TYPE_SIZE (TREE_TYPE (exp)), bitsize) != 0 /* Except for initialization of full bytes from a CONSTRUCTOR, which diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt index 45428d8..b1b4641 100644 --- a/gcc/fortran/lang.opt +++ b/gcc/fortran/lang.opt @@ -686,6 +686,10 @@ funderscoring Fortran Var(flag_underscoring) Init(1) Append underscores to externally visible names. +funknown-commons +Fortran Var(flag_unknown_commons) +Treat arrays in common blocks as having unknown size. + fwhole-file Fortran Ignore Does nothing. Preserved for backward compatibility. diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c index 21d1928..bf99c56 100644 --- a/gcc/fortran/trans-common.c +++ b/gcc/fortran/trans-common.c @@ -284,8 +284,41 @@ build_field (segment_info *h, tree union_type, record_layout_info rli) unsigned HOST_WIDE_INT desired_align, known_align; name = get_identifier (h->sym->name); + + gcc_assert (TYPE_P (h->field)); + tree size = TYPE_SIZE (h->field); + tree size_unit = TYPE_SIZE_UNIT (h->field); + if (GFC_ARRAY_TYPE_P (h->field) + && !h->sym->value + && flag_unknown_commons) + { + gcc_assert (!GFC_DESCRIPTOR_TYPE_P (h->field)); + gcc_assert (TREE_CODE (h->field) == ARRAY_TYPE); + + /* Could be merged at link time with a larger array whose size we don't + know. */ + static tree range_type = NULL_TREE; + if (!range_type) + range_type = build_range_type (gfc_array_index_type, + gfc_index_zero_node, NULL_TREE); + tree type = copy_node (h->field); + TYPE_DOMAIN (type) = range_type; + TYPE_SIZE (type) = NULL_TREE; + TYPE_SIZE_UNIT (type) = NULL_TREE; + SET_TYPE_MODE (type, BLKmode); + gcc_assert (TYPE_CANONICAL (h->field) == h->field); + gcc_assert (TYPE_MAIN_VARIANT (h->field) == h->field); + TYPE_CANONICAL (type) = type; + TYPE_MAIN_VARIANT (type) = type; + layout_type (type); + h->field = type; + } + field = build_decl (h->sym->declared_at.lb->location, FIELD_DECL, name, h->field); + DECL_SIZE (field) = size; + DECL_SIZE_UNIT (field) = size_unit; + known_align = (offset & -offset) * BITS_PER_UNIT; if (known_align == 0 || known_align > BIGGEST_ALIGNMENT) known_align = BIGGEST_ALIGNMENT; @@ -703,6 +736,8 @@ create_common (gfc_common_head *com, segment_info *head, bool saw_equiv) VAR_DECL, DECL_NAME (s->field), TREE_TYPE (s->field)); TREE_STATIC (var_decl) = TREE_STATIC (decl); + DECL_SIZE (var_decl) = DECL_SIZE (s->field); + DECL_SIZE_UNIT (var_decl) = DECL_SIZE_UNIT (s->field); /* Mark the variable as used in order to avoid warnings about unused variables. */ TREE_USED (var_decl) = 1; 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..340c04c 100644 --- a/gcc/tree-dfa.c +++ b/gcc/tree-dfa.c @@ -617,7 +617,11 @@ get_ref_base_and_extent (tree exp, HOST_WIDE_INT *poffset, if (maxsize == -1 && DECL_SIZE (exp) && TREE_CODE (DECL_SIZE (exp)) == INTEGER_CST) - maxsize = wi::to_offset (DECL_SIZE (exp)) - bit_offset; + { + maxsize = wi::to_offset (DECL_SIZE (exp)) - bit_offset; + if (maxsize == bitsize) + maxsize = -1; + } } else if (CONSTANT_CLASS_P (exp)) { diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c index 4c0e135..f8e8dce 100644 --- a/gcc/tree-vect-data-refs.c +++ b/gcc/tree-vect-data-refs.c @@ -3412,6 +3412,14 @@ again: return false; } + if (TYPE_SIZE (TREE_TYPE (DR_REF (dr))) == NULL_TREE) + { + if (dump_enabled_p ()) + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, + "not vectorized: data-ref of unknown size\n"); + return false; + } + stmt = DR_STMT (dr); stmt_info = vinfo_for_stmt (stmt);