| Message ID | ora8ut3oh6.fsf@livre.home |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 18, 2015 at 9:37 AM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Jul 16, 2015, Alexandre Oliva <aoliva@redhat.com> wrote: > >> So, I decided to run a ppc64le-linux-gnu bootstrap, just in case, and >> there are issues with split complex parms that caused go and fortran >> libs to fail the build. > > This incremental patch, along with the previously-posted patches, fix > split complex args handling with preassigned args RTL, and enables > ppc64le-linux-gnu bootstrap to succeed. > > I'm not particularly happy with the abuse of DECL_CONTEXT to recognize > split complex args and leave their RTL alone, but that was the best that > occurred to me. Any other suggestions? > > Is the combined patch ok, assuming further (re)testing of embedded > targets passes? > > for gcc/ChangeLog (to be integrated with the approved patches) > > * function.c (split_complex_args): Take assign_parm_data_all > argument. Pass it to rtl_for_parm. Set up rtl and context > for split args. > (assign_parms_augmented_arg_list): Adjust. > (maybe_reset_rtl_for_parm): Recognize split complex args. > * stor-layout.c (layout_decl): Don't set mem attributes of > non-MEMs. > --- > gcc/function.c | 39 +++++++++++++++++++++++++++++++++++++-- > gcc/stor-layout.c | 3 ++- > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/gcc/function.c b/gcc/function.c > index 753d889..6fba001 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -151,6 +151,8 @@ static bool contains (const_rtx, hash_table<insn_cache_hasher> *); > static void prepare_function_start (void); > 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); > + > > /* Stack of nested functions. */ > /* Keep track of the cfun stack. */ > @@ -2267,7 +2269,7 @@ assign_parms_initialize_all (struct assign_parm_data_all *all) > needed, else the old list. */ > > static void > -split_complex_args (vec<tree> *args) > +split_complex_args (struct assign_parm_data_all *all, vec<tree> *args) > { > unsigned i; > tree p; > @@ -2278,6 +2280,7 @@ split_complex_args (vec<tree> *args) > if (TREE_CODE (type) == COMPLEX_TYPE > && targetm.calls.split_complex_arg (type)) > { > + tree cparm = p; > tree decl; > tree subtype = TREE_TYPE (type); > bool addressable = TREE_ADDRESSABLE (p); > @@ -2296,6 +2299,9 @@ split_complex_args (vec<tree> *args) > DECL_ARTIFICIAL (p) = addressable; > DECL_IGNORED_P (p) = addressable; > TREE_ADDRESSABLE (p) = 0; > + /* Reset the RTL before layout_decl, or it may change the > + mode of the RTL of the original argument copied to P. */ > + SET_DECL_RTL (p, NULL_RTX); > layout_decl (p, 0); > (*args)[i] = p; > > @@ -2307,6 +2313,25 @@ split_complex_args (vec<tree> *args) > DECL_IGNORED_P (decl) = addressable; > layout_decl (decl, 0); > args->safe_insert (++i, decl); > + > + /* If we are assigning parameters for a function, rather > + than for a call, propagate the RTL of the complex parm to > + the split declarations, and set their contexts so that > + maybe_reset_rtl_for_parm can recognize them and refrain > + from resetting their RTL. */ > + if (cfun->gimple_df) If the cfun->gimple_df check is to decide whether this is a call or a function then no, this can't work reliably. What is this test for else? You pass another argument to split_complex_arg, so why not pass in a bool on whether we split it for this or the other case? Richard. > + { > + rtx rtl = rtl_for_parm (all, cparm); > + gcc_assert (!rtl || GET_CODE (rtl) == CONCAT); > + if (rtl) > + { > + SET_DECL_RTL (p, XEXP (rtl, 0)); > + SET_DECL_RTL (decl, XEXP (rtl, 1)); > + > + DECL_CONTEXT (p) = cparm; > + DECL_CONTEXT (decl) = cparm; > + } > + } > } > } > } > @@ -2369,7 +2394,7 @@ assign_parms_augmented_arg_list (struct assign_parm_data_all *all) > > /* If the target wants to split complex arguments into scalars, do so. */ > if (targetm.calls.split_complex_arg) > - split_complex_args (&fnargs); > + split_complex_args (all, &fnargs); > > return fnargs; > } > @@ -2823,6 +2848,16 @@ maybe_reset_rtl_for_parm (tree parm) > { > gcc_assert (TREE_CODE (parm) == PARM_DECL > || TREE_CODE (parm) == RESULT_DECL); > + > + /* This is a split complex parameter, and its context was set to its > + original PARM_DECL in split_complex_args so that we could > + recognize it here and not reset its RTL. */ > + if (DECL_CONTEXT (parm) && TREE_CODE (DECL_CONTEXT (parm)) == PARM_DECL) > + { > + DECL_CONTEXT (parm) = DECL_CONTEXT (DECL_CONTEXT (parm)); > + return; > + } > + > if ((flag_tree_coalesce_vars > || (DECL_RTL_SET_P (parm) && DECL_RTL (parm) == pc_rtx)) > && is_gimple_reg (parm)) > diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c > index 0d4f4a4..288227a 100644 > --- a/gcc/stor-layout.c > +++ b/gcc/stor-layout.c > @@ -794,7 +794,8 @@ layout_decl (tree decl, unsigned int known_align) > { > PUT_MODE (rtl, DECL_MODE (decl)); > SET_DECL_RTL (decl, 0); > - set_mem_attributes (rtl, decl, 1); > + if (MEM_P (rtl)) > + set_mem_attributes (rtl, decl, 1); > SET_DECL_RTL (decl, rtl); > } > } > > > -- > 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
On Jul 21, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > On Sat, Jul 18, 2015 at 9:37 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >> On Jul 16, 2015, Alexandre Oliva <aoliva@redhat.com> wrote: >> + /* If we are assigning parameters for a function, rather >> + than for a call, propagate the RTL of the complex parm to >> + the split declarations, and set their contexts so that >> + maybe_reset_rtl_for_parm can recognize them and refrain >> + from resetting their RTL. */ >> + if (cfun->gimple_df) > If the cfun->gimple_df check is to decide whether this is a call or a function > then no, this can't work reliably. What is this test for else? That was the reason: call or function. > You pass another argument to split_complex_arg, so why not pass in a bool > on whether we split it for this or the other case? There's only one call to split_complex_args. I'll try to figure out where the paths converge and see if it's reasonable to pass an argument all the way to tell the two cases apart. Thanks for the suggestion,
On Jul 21, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > On Sat, Jul 18, 2015 at 9:37 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >> + if (cfun->gimple_df) > If the cfun->gimple_df check is to decide whether this is a call or a function > then no, this can't work reliably. What is this test for else? It turns out it's not call or function, as I thought at first, but gimplifying or expanding the function. split_complex_args is not used for calls. So the above might actually work (minus the misleading comments I wrote), and I think it's cleaner than adding a bool expanding_p arg to split_complex_args and assign_parms_augmented_arg_list, called from gimplify_parameters (during gimplification of a function) and assign_parms (during its expansion). Do you agree, or would you prefer the explicit argument?
On Wed, Jul 22, 2015 at 7:33 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Jul 21, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > >> On Sat, Jul 18, 2015 at 9:37 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >>> + if (cfun->gimple_df) > >> If the cfun->gimple_df check is to decide whether this is a call or a function >> then no, this can't work reliably. What is this test for else? > > It turns out it's not call or function, as I thought at first, but > gimplifying or expanding the function. split_complex_args is not used > for calls. So the above might actually work (minus the misleading > comments I wrote), and I think it's cleaner than adding a bool > expanding_p arg to split_complex_args and > assign_parms_augmented_arg_list, called from gimplify_parameters (during > gimplification of a function) and assign_parms (during its expansion). > Do you agree, or would you prefer the explicit argument? Hmm, ok. Does using if (currently_expanding_to_rtl) work? I think it's slightly more descriptive. Ok with that change. Thanks, Richard. > -- > 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
diff --git a/gcc/function.c b/gcc/function.c index 753d889..6fba001 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -151,6 +151,8 @@ static bool contains (const_rtx, hash_table<insn_cache_hasher> *); static void prepare_function_start (void); 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); + /* Stack of nested functions. */ /* Keep track of the cfun stack. */ @@ -2267,7 +2269,7 @@ assign_parms_initialize_all (struct assign_parm_data_all *all) needed, else the old list. */ static void -split_complex_args (vec<tree> *args) +split_complex_args (struct assign_parm_data_all *all, vec<tree> *args) { unsigned i; tree p; @@ -2278,6 +2280,7 @@ split_complex_args (vec<tree> *args) if (TREE_CODE (type) == COMPLEX_TYPE && targetm.calls.split_complex_arg (type)) { + tree cparm = p; tree decl; tree subtype = TREE_TYPE (type); bool addressable = TREE_ADDRESSABLE (p); @@ -2296,6 +2299,9 @@ split_complex_args (vec<tree> *args) DECL_ARTIFICIAL (p) = addressable; DECL_IGNORED_P (p) = addressable; TREE_ADDRESSABLE (p) = 0; + /* Reset the RTL before layout_decl, or it may change the + mode of the RTL of the original argument copied to P. */ + SET_DECL_RTL (p, NULL_RTX); layout_decl (p, 0); (*args)[i] = p; @@ -2307,6 +2313,25 @@ split_complex_args (vec<tree> *args) DECL_IGNORED_P (decl) = addressable; layout_decl (decl, 0); args->safe_insert (++i, decl); + + /* If we are assigning parameters for a function, rather + than for a call, propagate the RTL of the complex parm to + the split declarations, and set their contexts so that + maybe_reset_rtl_for_parm can recognize them and refrain + from resetting their RTL. */ + if (cfun->gimple_df) + { + rtx rtl = rtl_for_parm (all, cparm); + gcc_assert (!rtl || GET_CODE (rtl) == CONCAT); + if (rtl) + { + SET_DECL_RTL (p, XEXP (rtl, 0)); + SET_DECL_RTL (decl, XEXP (rtl, 1)); + + DECL_CONTEXT (p) = cparm; + DECL_CONTEXT (decl) = cparm; + } + } } } } @@ -2369,7 +2394,7 @@ assign_parms_augmented_arg_list (struct assign_parm_data_all *all) /* If the target wants to split complex arguments into scalars, do so. */ if (targetm.calls.split_complex_arg) - split_complex_args (&fnargs); + split_complex_args (all, &fnargs); return fnargs; } @@ -2823,6 +2848,16 @@ maybe_reset_rtl_for_parm (tree parm) { gcc_assert (TREE_CODE (parm) == PARM_DECL || TREE_CODE (parm) == RESULT_DECL); + + /* This is a split complex parameter, and its context was set to its + original PARM_DECL in split_complex_args so that we could + recognize it here and not reset its RTL. */ + if (DECL_CONTEXT (parm) && TREE_CODE (DECL_CONTEXT (parm)) == PARM_DECL) + { + DECL_CONTEXT (parm) = DECL_CONTEXT (DECL_CONTEXT (parm)); + return; + } + if ((flag_tree_coalesce_vars || (DECL_RTL_SET_P (parm) && DECL_RTL (parm) == pc_rtx)) && is_gimple_reg (parm)) diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c index 0d4f4a4..288227a 100644 --- a/gcc/stor-layout.c +++ b/gcc/stor-layout.c @@ -794,7 +794,8 @@ layout_decl (tree decl, unsigned int known_align) { PUT_MODE (rtl, DECL_MODE (decl)); SET_DECL_RTL (decl, 0); - set_mem_attributes (rtl, decl, 1); + if (MEM_P (rtl)) + set_mem_attributes (rtl, decl, 1); SET_DECL_RTL (decl, rtl); } }