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 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).

> +  if (TARGET_64BIT
> +      && (call_used_regs[SI_REG]
> +	  == (cfun->machine->call_abi == MS_ABI)))
> +    reinit_regs ();
> +}
> +
>  
>  /* Return true if this goes in large data/bss.  */
>  
> @@ -7395,17 +7412,6 @@ ix86_call_abi_override (const_tree fndecl)
>    cfun->machine->call_abi = ix86_function_abi (fndecl);
>  }
>  
> -/* 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.  */
> -static void
> -ix86_maybe_switch_abi (void)
> -{
> -  if (TARGET_64BIT &&
> -      call_used_regs[SI_REG] == (cfun->machine->call_abi == MS_ABI))
> -    reinit_regs ();
> -}
> -
>  /* Return 1 if pseudo register should be created and used to hold
>     GOT address for PIC code.  */
>  bool
> @@ -53802,9 +53808,6 @@ ix86_operands_ok_for_move_multiple (rtx *operands, bool load,
>  #undef TARGET_CAN_INLINE_P
>  #define TARGET_CAN_INLINE_P ix86_can_inline_p
>  
> -#undef TARGET_EXPAND_TO_RTL_HOOK
> -#define TARGET_EXPAND_TO_RTL_HOOK ix86_maybe_switch_abi
> -
>  #undef TARGET_LEGITIMATE_ADDRESS_P
>  #define TARGET_LEGITIMATE_ADDRESS_P ix86_legitimate_address_p
>  
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)