From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84567 invoked by alias); 19 Aug 2015 10:06:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 84557 invoked by uid 89); 19 Aug 2015 10:06:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-io0-f170.google.com Received: from mail-io0-f170.google.com (HELO mail-io0-f170.google.com) (209.85.223.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 19 Aug 2015 10:05:59 +0000 Received: by iodv127 with SMTP id v127so2953343iod.3 for ; Wed, 19 Aug 2015 03:05:57 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.107.167.199 with SMTP id q190mr14082170ioe.119.1439978757199; Wed, 19 Aug 2015 03:05:57 -0700 (PDT) Received: by 10.107.32.140 with HTTP; Wed, 19 Aug 2015 03:05:56 -0700 (PDT) In-Reply-To: References: <20150723203112.GB27818@gate.crashing.org> <20150810082355.GA31149@arm.com> <55C8BFC3.3030603@redhat.com> Date: Wed, 19 Aug 2015 10:17:00 -0000 Message-ID: Subject: Re: [PR64164] drop copyrename, integrate into expand From: Richard Biener To: Alexandre Oliva Cc: Christophe Lyon , Andreas Schwab , GCC Patches , Patrick Marlier , Jeff Law , James Greenhalgh , "H.J. Lu" , Segher Boessenkool , David Edelsohn , Eric Botcazou Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg01057.txt.bz2 On Wed, Aug 19, 2015 at 8:45 AM, Alexandre Oliva wrote: > On Aug 18, 2015, Alexandre Oliva wrote: > >>>> On Aug 17, 2015, Christophe Lyon wrote: >>>>> Since this was committed (r226901), I can see that the compiler build >>>>> fails for armeb targets, when building libgcc: > >> This patch fixes this particular case. I'll also add this configuration >> to the cross build tests I'm going to rerun shortly, before submitting a >> followup formally, to see whether other non-MEM mems need to be handled >> explicitly. > > On Aug 17, 2015, Andreas Schwab wrote: > >> Andreas Schwab writes: > >>> Alexandre Oliva writes: >>> >>>> Would you be so kind as to give it a spin on a m68k native? TIA, >>> >>> I tried it on ia64, and it falls flat on the floor. > >> It fixes the m68k failures, though. > > On Aug 17, 2015, Alexandre Oliva wrote: > >>> I tried it on ia64, and it falls flat on the floor. > >> Doh, I see a logic flaw in the patch I posted. > > There were other shortcomings in the snippets I posted before, revealed > by testing on on various other targets: remaining BLKmode asserts, > failure to deal with parms without a default def and split complex args > with an unassigned stack address. This patch fixes them all. > > It was regstrapped on x86_64-linux-gnu, i686-linux-gnu, ppc64-linux-gnu, > ppc64el-linux-gnu, and further tested with a compile-only 'make all' on > a binutils+gcc+newlib tree on all tens of cross targets mentioned > before, plus the armeb configuration Christophe mentioned. > > Ok to install? Ok. Thanks, Richard. > > [PR64164] fix regressions reported on m68k and armeb > > From: Alexandre Oliva > > Defer stack slot address assignment for all parms that can't live in > pseudos, and accept pseudos assignments in assign_param_setup_block. > > for gcc/ChangeLog > > PR rtl-optimization/64164 > * cfgexpand.c (parm_maybe_byref_p): Renamed to... > (parm_in_stack_slot_p): ... this. Disregard mode, what > matters is whether the parm will live in a pseudo or a stack > slot. > (expand_one_ssa_partition): Deal with params without a default > def. Disregard mode. > * cfgexpand.h: Renamed function declaration. > * tree-ssa-coalesce.c: Adjust. > * function.c (split_complex_args): Allocate stack slot for > unassigned parms before splitting. > (parm_in_unassigned_mem_p): New. Use it instead of > parm_maybe_byref_p throughout this file. > (assign_parm_setup_block): Use it. Accept pseudos in the > expand-assigned rtl. > (assign_parm_setup_reg): Drop BLKmode requirement. > (assign_parm_setup_stack): Allocate and fill in the address of > unassigned MEM parms. > --- > gcc/cfgexpand.c | 44 ++++++++++++++++++++++------ > gcc/cfgexpand.h | 2 + > gcc/function.c | 74 ++++++++++++++++++++++++++++++++++++++++------- > gcc/tree-ssa-coalesce.c | 4 +-- > 4 files changed, 100 insertions(+), 24 deletions(-) > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index 0bc20f6..d567a87 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -172,17 +172,23 @@ leader_merge (tree cur, tree next) > return cur; > } > > -/* Return true if VAR is a PARM_DECL or a RESULT_DECL of type BLKmode. > +/* Return true if VAR is a PARM_DECL or a RESULT_DECL that ought to be > + assigned to a stack slot. We can't have expand_one_ssa_partition > + choose their address: the pseudo holding the address would be set > + up too late for assign_params to copy the parameter if needed. > + > Such parameters are likely passed as a pointer to the value, rather > than as a value, and so we must not coalesce them, nor allocate > stack space for them before determining the calling conventions for > - them. For their SSA_NAMEs, expand_one_ssa_partition emits RTL as > - MEMs with pc_rtx as the address, and then it replaces the pc_rtx > - with NULL so as to make sure the MEM is not used before it is > - adjusted in assign_parm_setup_reg. */ > + them. > + > + For their SSA_NAMEs, expand_one_ssa_partition emits RTL as MEMs > + with pc_rtx as the address, and then it replaces the pc_rtx with > + NULL so as to make sure the MEM is not used before it is adjusted > + in assign_parm_setup_reg. */ > > bool > -parm_maybe_byref_p (tree var) > +parm_in_stack_slot_p (tree var) > { > if (!var || VAR_P (var)) > return false; > @@ -190,7 +196,7 @@ parm_maybe_byref_p (tree var) > gcc_assert (TREE_CODE (var) == PARM_DECL > || TREE_CODE (var) == RESULT_DECL); > > - return TYPE_MODE (TREE_TYPE (var)) == BLKmode; > + return !use_register_for_decl (var); > } > > /* Return the partition of the default SSA_DEF for decl VAR. */ > @@ -1343,17 +1349,35 @@ expand_one_ssa_partition (tree var) > > if (!use_register_for_decl (var)) > { > - if (parm_maybe_byref_p (SSA_NAME_VAR (var)) > - && ssa_default_def_partition (SSA_NAME_VAR (var)) == part) > + /* We can't risk having the parm assigned to a MEM location > + whose address references a pseudo, for the pseudo will only > + be set up after arguments are copied to the stack slot. > + > + If the parm doesn't have a default def (e.g., because its > + incoming value is unused), then we want to let assign_params > + do the allocation, too. In this case we want to make sure > + SSA_NAMEs associated with the parm don't get assigned to more > + than one partition, lest we'd create two unassigned stac > + slots for the same parm, thus the assert at the end of the > + block. */ > + if (parm_in_stack_slot_p (SSA_NAME_VAR (var)) > + && (ssa_default_def_partition (SSA_NAME_VAR (var)) == part > + || !ssa_default_def (cfun, SSA_NAME_VAR (var)))) > { > expand_one_stack_var_at (var, pc_rtx, 0, 0); > rtx x = SA.partition_to_pseudo[part]; > gcc_assert (GET_CODE (x) == MEM); > - gcc_assert (GET_MODE (x) == BLKmode); > gcc_assert (XEXP (x, 0) == pc_rtx); > /* Reset the address, so that any attempt to use it will > ICE. It will be adjusted in assign_parm_setup_reg. */ > XEXP (x, 0) = NULL_RTX; > + /* If the RTL associated with the parm is not what we have > + just created, the parm has been split over multiple > + partitions. In order for this to work, we must have a > + default def for the parm, otherwise assign_params won't > + know what to do. */ > + gcc_assert (DECL_RTL_IF_SET (SSA_NAME_VAR (var)) == x > + || ssa_default_def (cfun, SSA_NAME_VAR (var))); > } > else if (defer_stack_allocation (var, true)) > add_stack_var (var); > diff --git a/gcc/cfgexpand.h b/gcc/cfgexpand.h > index 987cf356..d168672 100644 > --- a/gcc/cfgexpand.h > +++ b/gcc/cfgexpand.h > @@ -22,7 +22,7 @@ along with GCC; see the file COPYING3. If not see > > extern tree gimple_assign_rhs_to_tree (gimple); > extern HOST_WIDE_INT estimated_stack_frame_size (struct cgraph_node *); > -extern bool parm_maybe_byref_p (tree); > +extern bool parm_in_stack_slot_p (tree); > extern rtx get_rtl_for_parm_ssa_default_def (tree var); > > > diff --git a/gcc/function.c b/gcc/function.c > index 715c19f..222c76f 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -153,6 +153,7 @@ static void do_clobber_return_reg (rtx, void *); > static void do_use_return_reg (rtx, void *); > static rtx rtl_for_parm (struct assign_parm_data_all *, tree); > static void maybe_reset_rtl_for_parm (tree); > +static bool parm_in_unassigned_mem_p (tree, rtx); > > > /* Stack of nested functions. */ > @@ -2326,6 +2327,22 @@ split_complex_args (struct assign_parm_data_all *all, vec *args) > rtx rtl = rtl_for_parm (all, cparm); > if (rtl) > { > + /* If this is parm is unassigned, assign it now: the > + newly-created decls wouldn't expect the need for > + assignment, and if they were assigned > + independently, they might not end up in adjacent > + slots, so unsplit wouldn't be able to fill in the > + unassigned address of the complex MEM. */ > + if (parm_in_unassigned_mem_p (cparm, rtl)) > + { > + int align = STACK_SLOT_ALIGNMENT > + (TREE_TYPE (cparm), GET_MODE (rtl), MEM_ALIGN (rtl)); > + rtx loc = assign_stack_local > + (GET_MODE (rtl), GET_MODE_SIZE (GET_MODE (rtl)), > + align); > + XEXP (rtl, 0) = XEXP (loc, 0); > + } > + > SET_DECL_RTL (p, read_complex_part (rtl, false)); > SET_DECL_RTL (decl, read_complex_part (rtl, true)); > > @@ -2934,6 +2951,27 @@ assign_parm_setup_block_p (struct assign_parm_data_one *data) > return false; > } > > +/* Return true if FROM_EXPAND is a MEM with an address to be filled in > + by assign_params. This should be the case if, and only if, > + parm_in_stack_slot_p holds for the parm DECL that expanded to > + FROM_EXPAND, so we check that, too. */ > + > +static bool > +parm_in_unassigned_mem_p (tree decl, rtx from_expand) > +{ > + bool result = MEM_P (from_expand) && !XEXP (from_expand, 0); > + > + gcc_assert (result == parm_in_stack_slot_p (decl) > + /* Maybe it was already assigned. That's ok, especially > + for split complex args. */ > + || (!result && MEM_P (from_expand) > + && (XEXP (from_expand, 0) == virtual_stack_vars_rtx > + || (GET_CODE (XEXP (from_expand, 0)) == PLUS > + && XEXP (XEXP (from_expand, 0), 0) == virtual_stack_vars_rtx)))); > + > + return result; > +} > + > /* A subroutine of assign_parms. Arrange for the parameter to be > present and valid in DATA->STACK_RTL. */ > > @@ -2956,8 +2994,7 @@ assign_parm_setup_block (struct assign_parm_data_all *all, > { > DECL_ALIGN (parm) = MAX (DECL_ALIGN (parm), BITS_PER_WORD); > rtx from_expand = rtl_for_parm (all, parm); > - if (from_expand && (!parm_maybe_byref_p (parm) > - || XEXP (from_expand, 0) != NULL_RTX)) > + if (from_expand && !parm_in_unassigned_mem_p (parm, from_expand)) > stack_parm = copy_rtx (from_expand); > else > { > @@ -2968,8 +3005,7 @@ assign_parm_setup_block (struct assign_parm_data_all *all, > if (from_expand) > { > gcc_assert (GET_CODE (stack_parm) == MEM); > - gcc_assert (GET_CODE (from_expand) == MEM); > - gcc_assert (XEXP (from_expand, 0) == NULL_RTX); > + gcc_assert (parm_in_unassigned_mem_p (parm, from_expand)); > XEXP (from_expand, 0) = XEXP (stack_parm, 0); > PUT_MODE (from_expand, GET_MODE (stack_parm)); > stack_parm = copy_rtx (from_expand); > @@ -3017,6 +3053,11 @@ assign_parm_setup_block (struct assign_parm_data_all *all, > else if (size == 0) > ; > > + /* MEM may be a REG if coalescing assigns the param's partition > + to a pseudo. */ > + else if (REG_P (mem)) > + emit_move_insn (mem, entry_parm); > + > /* If SIZE is that of a mode no bigger than a word, just use > that mode's store operation. */ > else if (size <= UNITS_PER_WORD) > @@ -3121,7 +3162,7 @@ assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm, > if (GET_MODE (parmreg) != promoted_nominal_mode) > parmreg = gen_lowpart (promoted_nominal_mode, parmreg); > } > - else if (!from_expand || parm_maybe_byref_p (parm)) > + else if (!from_expand || parm_in_unassigned_mem_p (parm, from_expand)) > { > parmreg = gen_reg_rtx (promoted_nominal_mode); > if (!DECL_ARTIFICIAL (parm)) > @@ -3131,7 +3172,6 @@ assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm, > { > gcc_assert (data->passed_pointer); > gcc_assert (GET_CODE (from_expand) == MEM > - && GET_MODE (from_expand) == BLKmode > && XEXP (from_expand, 0) == NULL_RTX); > XEXP (from_expand, 0) = parmreg; > } > @@ -3349,7 +3389,7 @@ assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm, > did_conversion = true; > } > else if (GET_MODE (parmreg) == BLKmode) > - gcc_assert (parm_maybe_byref_p (parm)); > + gcc_assert (parm_in_stack_slot_p (parm)); > else > emit_move_insn (parmreg, src); > > @@ -3455,12 +3495,15 @@ assign_parm_setup_stack (struct assign_parm_data_all *all, tree parm, > if (data->entry_parm != data->stack_parm) > { > rtx src, dest; > + rtx from_expand = NULL_RTX; > > if (data->stack_parm == 0) > { > - rtx x = data->stack_parm = rtl_for_parm (all, parm); > - if (x) > - gcc_assert (GET_MODE (x) == GET_MODE (data->entry_parm)); > + from_expand = rtl_for_parm (all, parm); > + if (from_expand) > + gcc_assert (GET_MODE (from_expand) == GET_MODE (data->entry_parm)); > + if (from_expand && !parm_in_unassigned_mem_p (parm, from_expand)) > + data->stack_parm = from_expand; > } > > if (data->stack_parm == 0) > @@ -3472,7 +3515,16 @@ assign_parm_setup_stack (struct assign_parm_data_all *all, tree parm, > = assign_stack_local (GET_MODE (data->entry_parm), > GET_MODE_SIZE (GET_MODE (data->entry_parm)), > align); > - set_mem_attributes (data->stack_parm, parm, 1); > + if (!from_expand) > + set_mem_attributes (data->stack_parm, parm, 1); > + else > + { > + gcc_assert (GET_CODE (data->stack_parm) == MEM); > + gcc_assert (parm_in_unassigned_mem_p (parm, from_expand)); > + XEXP (from_expand, 0) = XEXP (data->stack_parm, 0); > + PUT_MODE (from_expand, GET_MODE (data->stack_parm)); > + data->stack_parm = copy_rtx (from_expand); > + } > } > > dest = validize_mem (copy_rtx (data->stack_parm)); > diff --git a/gcc/tree-ssa-coalesce.c b/gcc/tree-ssa-coalesce.c > index 08ce72c..6468012 100644 > --- a/gcc/tree-ssa-coalesce.c > +++ b/gcc/tree-ssa-coalesce.c > @@ -1386,8 +1386,8 @@ gimple_can_coalesce_p (tree name1, tree name2) > because it may be passed by reference. */ > return ((!var1 || VAR_P (var1)) && (!var2 || VAR_P (var2))) > || (/* The case var1 == var2 is already covered above. */ > - !parm_maybe_byref_p (var1) > - && !parm_maybe_byref_p (var2) > + !parm_in_stack_slot_p (var1) > + && !parm_in_stack_slot_p (var2) > && promote_ssa_mode (name1, NULL) == promote_ssa_mode (name2, NULL)); > } > > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer