Re: [PATCH] PR target/67850: Wrong call_used_regs used in aggregate_value_p

[Richard Biener <rguenther at suse dot de>]

On Tue, 6 Oct 2015, H.J. Lu wrote:

> On Tue, Oct 6, 2015 at 8:01 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Tue, Oct 6, 2015 at 6:39 AM, Richard Biener <rguenther@suse.de> wrote:
> >> On Tue, 6 Oct 2015, H.J. Lu wrote:
> >>
> >>> On Tue, Oct 06, 2015 at 02:30:59PM +0200, Richard Biener wrote:
> >>> > On Tue, Oct 6, 2015 at 1:43 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> >>> > > Since targetm.expand_to_rtl_hook may be called to switch ABI, it should
> >>> > > be called for each function before expanding to RTL.  Otherwise, we may
> >>> > > use the stale information from compilation of the previous function.
> >>> > > aggregate_value_p uses call_used_regs.  aggregate_value_p is used by
> >>> > > IPA and return value optimization, which are called before
> >>> > > pass_expand::execute after RTL expansion starts.  We need to call
> >>> > > targetm.expand_to_rtl_hook early enough in cgraph_node::expand to make
> >>> > > sure that everything is in sync when RTL expansion starts.
> >>> > >
> >>> > > Tested on Linux/x86-64.  OK for trunk?
> >>> >
> >>> > Hmm, I think set_cfun hook should handle this.  expand_to_rtl_hook shouldn't
> >>> > mess with per-function stuff.
> >>> >
> >>> > Richard.
> >>> >
> >>>
> >>> I am testig this patch.  OK for trunk if there is no regresion?
> >>>
> >>>
> >>> H.J.
> >>> --
> >>> ix86_maybe_switch_abi is called to late during RTL expansion and we
> >>> use the stale information from compilation of the previous function.
> >>> aggregate_value_p uses call_used_regs.  aggregate_value_p is used by
> >>> IPA and return value optimization, which are called before
> >>> pass_expand::execute after RTL expansion starts.  Instead,
> >>> ix86_maybe_switch_abi should be merged with ix86_set_current_function.
> >>>
> >>>       PR target/67850
> >>>       * config/i386/i386.c (ix86_set_current_function): Renamed
> >>>       to ...
> >>>       (ix86_set_current_function_1): This.
> >>>       (ix86_set_current_function): New. incorporate old
> >>>       ix86_set_current_function and ix86_maybe_switch_abi.
> >>>       (ix86_maybe_switch_abi): Removed.
> >>>       (TARGET_EXPAND_TO_RTL_HOOK): Likewise.
> >>> ---
> >>>  gcc/config/i386/i386.c | 33 ++++++++++++++++++---------------
> >>>  1 file changed, 18 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> >>> index d59b59b..a0adf3d 100644
> >>> --- a/gcc/config/i386/i386.c
> >>> +++ b/gcc/config/i386/i386.c
> >>> @@ -6222,7 +6222,7 @@ ix86_reset_previous_fndecl (void)
> >>>     FNDECL.  The argument might be NULL to indicate processing at top
> >>>     level, outside of any function scope.  */
> >>>  static void
> >>> -ix86_set_current_function (tree fndecl)
> >>> +ix86_set_current_function_1 (tree fndecl)
> >>>  {
> >>>    /* Only change the context if the function changes.  This hook is called
> >>>       several times in the course of compiling a function, and we don't want to
> >>> @@ -6262,6 +6262,23 @@ ix86_set_current_function (tree fndecl)
> >>>    ix86_previous_fndecl = fndecl;
> >>>  }
> >>>
> >>> +static void
> >>> +ix86_set_current_function (tree fndecl)
> >>> +{
> >>> +  ix86_set_current_function_1 (fndecl);
> >>> +
> >>> +  if (!cfun)
> >>> +    return;
> >>
> >> I think you want to test !fndecl here.  Why split this out at all?
> >> The ix86_previous_fndecl caching should still work, no?
> >>
> >>> +  /* 64-bit MS and SYSV ABI have different set of call used registers.
> >>> +     Avoid expensive re-initialization of init_regs each time we switch
> >>> +     function context since this is needed only during RTL expansion.  */
> >>
> >> The comment is now wrong (and your bug shows it was wrong previously).
> >>
> >
> > Here is the updated patch.  OK for master if there is no
> > regression on Linux/x86-64?
> >
> 
> There is no regression.  OK for trunk?

Ok with me but I defer to Uros for the approval.

Richard.