| Message ID | 20110607063150.GY17079@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 7, 2011 at 8:31 AM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > While for the trunk I hope Michael will finalize a much better fix, > this patch provides a quick workaround for 4.6 branch. > > In particular, I'd like to avoid reverting the > http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01442.html > patch, because if GIMPLE passes don't do any significant code motion, > sharing the stack slots e.g. for Fortran I/O parameter block helps quite a > bit. So, what this patch attempts to do is (only for the cases which > changed by the above mentioned patch) make the stack slot sharing > conditional on no significant code motion (loop {,complete} unrolling and other > kind of bb duplication). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for 4.6 branch? > Or is it way too ugly and should I rather revert there my patch? Ugh ... > 2011-06-07 Jakub Jelinek <jakub@redhat.com> > > PR fortran/49103 > * tree.h (DECL_NONSHAREABLE): Define. > (struct tree_decl_common): Change decl_common_unused to > decl_nonshareable_flag. > * cfgexpand.c (expand_used_vars_for_block, clear_tree_used): > Ignore vars with DECL_NONSHAREABLE bit set. > * tree-cfg.c (gimple_duplicate_bb): Set DECL_NONSHAREABLE > on stores to automatic aggregate vars. > > * gfortran.dg/pr49103.f90: New test. > > --- gcc/tree.h.jj 2011-03-14 14:12:15.000000000 +0100 > +++ gcc/tree.h 2011-05-31 14:05:34.000000000 +0200 > @@ -1330,6 +1330,10 @@ extern void omp_clause_range_check_faile > #define DECL_READ_P(NODE) \ > (TREE_CHECK2 (NODE, VAR_DECL, PARM_DECL)->decl_common.decl_read_flag) > > +#define DECL_NONSHAREABLE(NODE) \ > + (TREE_CHECK2 (NODE, VAR_DECL, \ > + RESULT_DECL)->decl_common.decl_nonshareable_flag) > + > /* In a CALL_EXPR, means that the call is the jump from a thunk to the > thunked-to function. */ > #define CALL_FROM_THUNK_P(NODE) (CALL_EXPR_CHECK (NODE)->base.protected_flag) > @@ -2787,8 +2791,9 @@ struct GTY(()) tree_decl_common { > being set. */ > unsigned decl_read_flag : 1; > > - /* Padding so that 'off_align' can be on a 32-bit boundary. */ > - unsigned decl_common_unused : 1; > + /* In VAR_DECL or RESULT_DECL set when significant code movement precludes > + attempting to share the stack slot with some other variable. */ > + unsigned decl_nonshareable_flag : 1; > > /* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. */ > unsigned int off_align : 8; > --- gcc/cfgexpand.c.jj 2011-05-04 10:46:52.000000000 +0200 > +++ gcc/cfgexpand.c 2011-05-31 14:08:36.000000000 +0200 > @@ -1134,7 +1134,9 @@ expand_used_vars_for_block (tree block, > > /* Expand all variables at this level. */ > for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t)) > - if (TREE_USED (t)) > + if (TREE_USED (t) > + && ((TREE_CODE (t) != VAR_DECL && TREE_CODE (t) != RESULT_DECL) > + || !DECL_NONSHAREABLE (t))) > expand_one_var (t, toplevel, true); > > this_sv_num = stack_vars_num; > @@ -1167,6 +1169,8 @@ clear_tree_used (tree block) > > for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t)) > /* if (!TREE_STATIC (t) && !DECL_EXTERNAL (t)) */ > + if ((TREE_CODE (t) != VAR_DECL && TREE_CODE (t) != RESULT_DECL) > + || !DECL_NONSHAREABLE (t)) > TREE_USED (t) = 0; > > for (t = BLOCK_SUBBLOCKS (block); t ; t = BLOCK_CHAIN (t)) > --- gcc/tree-cfg.c.jj 2011-03-14 14:12:15.000000000 +0100 > +++ gcc/tree-cfg.c 2011-05-31 15:22:14.000000000 +0200 > @@ -5117,6 +5117,7 @@ gimple_duplicate_bb (basic_block bb) > { > def_operand_p def_p; > ssa_op_iter op_iter; > + tree lhs; > > stmt = gsi_stmt (gsi); > if (gimple_code (stmt) == GIMPLE_LABEL) > @@ -5130,6 +5131,29 @@ gimple_duplicate_bb (basic_block bb) > maybe_duplicate_eh_stmt (copy, stmt); > gimple_duplicate_stmt_histograms (cfun, copy, cfun, stmt); > > + /* When copying around a stmt writing into a local non-user > + aggregate, make sure it won't share stack slot with other > + vars. */ > + lhs = gimple_get_lhs (stmt); > + if (lhs > + && (handled_component_p (lhs) > + || TREE_CODE (lhs) == MEM_REF > + || TREE_CODE (lhs) == TARGET_MEM_REF > + || TREE_CODE (lhs) == VAR_DECL > + || TREE_CODE (lhs) == RESULT_DECL)) > + { > + tree base = get_base_address (lhs); Probably easier and more complete to do if (lhs && TREE_CODE (lhs) != SSA_NAME) { tree base = get_base_address (lhs); I don't like the patch too much, but it looks reasonable. At least reverting your patch doesn't really fix anything. Any opinions from others? Thanks, Richard. > + if (base > + && (TREE_CODE (base) == VAR_DECL > + || TREE_CODE (base) == RESULT_DECL) > + && DECL_IGNORED_P (base) > + && !TREE_STATIC (base) > + && !DECL_EXTERNAL (base) > + && (TREE_CODE (base) != VAR_DECL > + || !DECL_HAS_VALUE_EXPR_P (base))) > + DECL_NONSHAREABLE (base) = 1; > + } > + > /* Create new names for all the definitions created by COPY and > add replacement mappings for each new name. */ > FOR_EACH_SSA_DEF_OPERAND (def_p, copy, op_iter, SSA_OP_ALL_DEFS) > --- gcc/testsuite/gfortran.dg/pr49103.f90.jj 2011-05-31 13:52:43.000000000 +0200 > +++ gcc/testsuite/gfortran.dg/pr49103.f90 2011-05-31 13:57:16.000000000 +0200 > @@ -0,0 +1,19 @@ > +! PR fortran/49103 > +! { dg-do run } > + integer :: a(2), b(2), i, j > + open (10, status='scratch') > + do j = 1, 2 > + a = (/ 0, 0 /) > + b = (/ 1, 1 /) > + do i = 1, 2 > + write (10, *) a > + write (10, *) b > + end do > + end do > + rewind (10) > + do i = 0, 7 > + read (10, *) a > + if (any (a .ne. mod (i, 2))) call abort > + end do > + close (10) > +end > > Jakub >
Hi, On Tue, 7 Jun 2011, Richard Guenther wrote: > > + tree base = get_base_address (lhs); > > Probably easier and more complete to do > > if (lhs && TREE_CODE (lhs) != SSA_NAME) > { > tree base = get_base_address (lhs); > > > I don't like the patch too much, but it looks reasonable. At least > reverting your patch doesn't really fix anything. > > Any opinions from others? It's ugly. But ... well, IMHO such hacks are acceptable for released branches. Ciao, Michael.
--- gcc/tree.h.jj 2011-03-14 14:12:15.000000000 +0100 +++ gcc/tree.h 2011-05-31 14:05:34.000000000 +0200 @@ -1330,6 +1330,10 @@ extern void omp_clause_range_check_faile #define DECL_READ_P(NODE) \ (TREE_CHECK2 (NODE, VAR_DECL, PARM_DECL)->decl_common.decl_read_flag) +#define DECL_NONSHAREABLE(NODE) \ + (TREE_CHECK2 (NODE, VAR_DECL, \ + RESULT_DECL)->decl_common.decl_nonshareable_flag) + /* In a CALL_EXPR, means that the call is the jump from a thunk to the thunked-to function. */ #define CALL_FROM_THUNK_P(NODE) (CALL_EXPR_CHECK (NODE)->base.protected_flag) @@ -2787,8 +2791,9 @@ struct GTY(()) tree_decl_common { being set. */ unsigned decl_read_flag : 1; - /* Padding so that 'off_align' can be on a 32-bit boundary. */ - unsigned decl_common_unused : 1; + /* In VAR_DECL or RESULT_DECL set when significant code movement precludes + attempting to share the stack slot with some other variable. */ + unsigned decl_nonshareable_flag : 1; /* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. */ unsigned int off_align : 8; --- gcc/cfgexpand.c.jj 2011-05-04 10:46:52.000000000 +0200 +++ gcc/cfgexpand.c 2011-05-31 14:08:36.000000000 +0200 @@ -1134,7 +1134,9 @@ expand_used_vars_for_block (tree block, /* Expand all variables at this level. */ for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t)) - if (TREE_USED (t)) + if (TREE_USED (t) + && ((TREE_CODE (t) != VAR_DECL && TREE_CODE (t) != RESULT_DECL) + || !DECL_NONSHAREABLE (t))) expand_one_var (t, toplevel, true); this_sv_num = stack_vars_num; @@ -1167,6 +1169,8 @@ clear_tree_used (tree block) for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t)) /* if (!TREE_STATIC (t) && !DECL_EXTERNAL (t)) */ + if ((TREE_CODE (t) != VAR_DECL && TREE_CODE (t) != RESULT_DECL) + || !DECL_NONSHAREABLE (t)) TREE_USED (t) = 0; for (t = BLOCK_SUBBLOCKS (block); t ; t = BLOCK_CHAIN (t)) --- gcc/tree-cfg.c.jj 2011-03-14 14:12:15.000000000 +0100 +++ gcc/tree-cfg.c 2011-05-31 15:22:14.000000000 +0200 @@ -5117,6 +5117,7 @@ gimple_duplicate_bb (basic_block bb) { def_operand_p def_p; ssa_op_iter op_iter; + tree lhs; stmt = gsi_stmt (gsi); if (gimple_code (stmt) == GIMPLE_LABEL) @@ -5130,6 +5131,29 @@ gimple_duplicate_bb (basic_block bb) maybe_duplicate_eh_stmt (copy, stmt); gimple_duplicate_stmt_histograms (cfun, copy, cfun, stmt); + /* When copying around a stmt writing into a local non-user + aggregate, make sure it won't share stack slot with other + vars. */ + lhs = gimple_get_lhs (stmt); + if (lhs + && (handled_component_p (lhs) + || TREE_CODE (lhs) == MEM_REF + || TREE_CODE (lhs) == TARGET_MEM_REF + || TREE_CODE (lhs) == VAR_DECL + || TREE_CODE (lhs) == RESULT_DECL)) + { + tree base = get_base_address (lhs); + if (base + && (TREE_CODE (base) == VAR_DECL + || TREE_CODE (base) == RESULT_DECL) + && DECL_IGNORED_P (base) + && !TREE_STATIC (base) + && !DECL_EXTERNAL (base) + && (TREE_CODE (base) != VAR_DECL + || !DECL_HAS_VALUE_EXPR_P (base))) + DECL_NONSHAREABLE (base) = 1; + } + /* Create new names for all the definitions created by COPY and add replacement mappings for each new name. */ FOR_EACH_SSA_DEF_OPERAND (def_p, copy, op_iter, SSA_OP_ALL_DEFS) --- gcc/testsuite/gfortran.dg/pr49103.f90.jj 2011-05-31 13:52:43.000000000 +0200 +++ gcc/testsuite/gfortran.dg/pr49103.f90 2011-05-31 13:57:16.000000000 +0200 @@ -0,0 +1,19 @@ +! PR fortran/49103 +! { dg-do run } + integer :: a(2), b(2), i, j + open (10, status='scratch') + do j = 1, 2 + a = (/ 0, 0 /) + b = (/ 1, 1 /) + do i = 1, 2 + write (10, *) a + write (10, *) b + end do + end do + rewind (10) + do i = 0, 7 + read (10, *) a + if (any (a .ne. mod (i, 2))) call abort + end do + close (10) +end
Hi! While for the trunk I hope Michael will finalize a much better fix, this patch provides a quick workaround for 4.6 branch. In particular, I'd like to avoid reverting the http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01442.html patch, because if GIMPLE passes don't do any significant code motion, sharing the stack slots e.g. for Fortran I/O parameter block helps quite a bit. So, what this patch attempts to do is (only for the cases which changed by the above mentioned patch) make the stack slot sharing conditional on no significant code motion (loop {,complete} unrolling and other kind of bb duplication). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for 4.6 branch? Or is it way too ugly and should I rather revert there my patch? 2011-06-07 Jakub Jelinek <jakub@redhat.com> PR fortran/49103 * tree.h (DECL_NONSHAREABLE): Define. (struct tree_decl_common): Change decl_common_unused to decl_nonshareable_flag. * cfgexpand.c (expand_used_vars_for_block, clear_tree_used): Ignore vars with DECL_NONSHAREABLE bit set. * tree-cfg.c (gimple_duplicate_bb): Set DECL_NONSHAREABLE on stores to automatic aggregate vars. * gfortran.dg/pr49103.f90: New test. Jakub