[PATCH] PR middle-end/67850: Wrong call_used_regs used in aggregate_value_p

[PATCH] PR middle-end/67850: Wrong call_used_regs used in aggregate_value_p

[H.J. Lu]

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?


H.J.
---
	PR middle-end/67850
	* cfgexpand.c (pass_expand::execute): Don't call
	targetm.expand_to_rtl_hook here.
	* cgraphunit.c (cgraph_node::expand): Call
	targetm.expand_to_rtl_hook here.
---
 gcc/cfgexpand.c  | 1 -
 gcc/cgraphunit.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 58e55d2..6891750 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6150,7 +6150,6 @@ pass_expand::execute (function *fun)
   /* Mark arrays indexed with non-constant indices with TREE_ADDRESSABLE.  */
   discover_nonconstant_array_refs ();
 
-  targetm.expand_to_rtl_hook ();
   crtl->stack_alignment_needed = STACK_BOUNDARY;
   crtl->max_used_stack_slot_alignment = STACK_BOUNDARY;
   crtl->stack_alignment_estimated = 0;
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 04a4d3f..537a089 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1973,6 +1973,9 @@ cgraph_node::expand (void)
 
   bitmap_obstack_initialize (&reg_obstack); /* FIXME, only at RTL generation*/
 
+  /* It may update call_used_regs, which is used by aggregate_value_p.  */
+  targetm.expand_to_rtl_hook ();
+
   execute_all_ipa_transforms ();
 
   /* Perform all tree transforms and optimizations.  */
-- 
2.4.3

Re: [PATCH] PR middle-end/67850: Wrong call_used_regs used in aggregate_value_p

[Richard Biener]

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.

>
> H.J.
> ---
>         PR middle-end/67850
>         * cfgexpand.c (pass_expand::execute): Don't call
>         targetm.expand_to_rtl_hook here.
>         * cgraphunit.c (cgraph_node::expand): Call
>         targetm.expand_to_rtl_hook here.
> ---
>  gcc/cfgexpand.c  | 1 -
>  gcc/cgraphunit.c | 3 +++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 58e55d2..6891750 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -6150,7 +6150,6 @@ pass_expand::execute (function *fun)
>    /* Mark arrays indexed with non-constant indices with TREE_ADDRESSABLE.  */
>    discover_nonconstant_array_refs ();
>
> -  targetm.expand_to_rtl_hook ();
>    crtl->stack_alignment_needed = STACK_BOUNDARY;
>    crtl->max_used_stack_slot_alignment = STACK_BOUNDARY;
>    crtl->stack_alignment_estimated = 0;
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index 04a4d3f..537a089 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -1973,6 +1973,9 @@ cgraph_node::expand (void)
>
>    bitmap_obstack_initialize (&reg_obstack); /* FIXME, only at RTL generation*/
>
> +  /* It may update call_used_regs, which is used by aggregate_value_p.  */
> +  targetm.expand_to_rtl_hook ();
> +
>    execute_all_ipa_transforms ();
>
>    /* Perform all tree transforms and optimizations.  */
> --
> 2.4.3
>

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

[H.J. Lu]

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;
+
+  /* 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.  */
+  if (TARGET_64BIT
+      && (call_used_regs[SI_REG]
+	  == (cfun->machine->call_abi == MS_ABI)))
+    reinit_regs ();
+}
+
 \f
 /* 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
 
-- 
2.4.3

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

[Richard Biener]

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 ();
> +}
> +
>  \f
>  /* 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)

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

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?

-- 
H.J.

[-- Attachment #2: 0001-Merge-ix86_maybe_switch_abi-with-ix86_set_current_fu.patch --]
[-- Type: text/x-patch, Size: 2468 bytes --]

From e5c618c9f951d6b9e2c534d4ace9225c6c991c75 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 6 Oct 2015 05:50:37 -0700
Subject: [PATCH] Merge ix86_maybe_switch_abi with ix86_set_current_function

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
ix86_maybe_switch_abi is called.  This patch merges ix86_maybe_switch_abi
with ix86_set_current_function.

	PR target/67850
	* config/i386/i386.c (ix86_maybe_switch_abi): Merged with ...
	(ix86_set_current_function): This.
	(TARGET_EXPAND_TO_RTL_HOOK): Removed.
---
 gcc/config/i386/i386.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 38953dd..c44f0af 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -6367,6 +6367,14 @@ ix86_set_current_function (tree fndecl)
 	TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
     }
   ix86_previous_fndecl = 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.  */
+  if (TARGET_64BIT
+      && (call_used_regs[SI_REG]
+	  == (cfun->machine->call_abi == MS_ABI)))
+    reinit_regs ();
 }
 
 \f
@@ -7502,17 +7510,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
@@ -53911,9 +53908,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
 
-- 
2.4.3

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

[H.J. Lu]

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?


-- 
H.J.

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

[Richard Biener]

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.

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

[Uros Bizjak]

On Wed, Oct 7, 2015 at 10:53 AM, Richard Biener <rguenther@suse.de> 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.

OK for mainline and release branches after a few days.

Thanks,
Uros.

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

[H.J. Lu]

On Wed, Oct 7, 2015 at 2:01 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Oct 7, 2015 at 10:53 AM, Richard Biener <rguenther@suse.de> 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.
>
> OK for mainline and release branches after a few days.
>

I backported it to GCC 5.  Backporting to 4.9 requires significant
change since ix86_set_current_function has changed since 4.9.
I have no plan to backport it to 4.9.


-- 
H.J.