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

[H.J. Lu]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Merge-ix86_maybe_switch_abi-with-ix86_set_current_fu.patch
Type: text/x-patch
Size: 2468 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151006/ece73f01/attachment.bin>