On Tue, Oct 6, 2015 at 8:01 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Tue, Oct 6, 2015 at 6:39 AM, Richard Biener <rguent...@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...@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.
