On Wed, Oct 7, 2015 at 10:53 AM, Richard Biener <[email protected]> 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.
