This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] PR target/67850: Wrong call_used_regs used in aggregate_value_p
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Uros Bizjak <ubizjak at gmail dot com>
- Cc: Richard Biener <rguenther at suse dot de>, Richard Biener <richard dot guenther at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Jeff Law <law at redhat dot com>
- Date: Mon, 12 Oct 2015 05:50:39 -0700
- Subject: Re: [PATCH] PR target/67850: Wrong call_used_regs used in aggregate_value_p
- Authentication-results: sourceware.org; auth=none
- References: <20151006114313 dot GA11910 at intel dot com> <CAFiYyc14wu3wziksJr3Gm38jD0xqad3d9raKnQh8cSbAXGyj1Q at mail dot gmail dot com> <20151006133051 dot GA25074 at gmail dot com> <alpine dot LSU dot 2 dot 11 dot 1510061532030 dot 6516 at zhemvz dot fhfr dot qr> <CAMe9rOopABY4==1zrXJohyYoC4jqeihMJC5abvtcpEDnM9ZLJw at mail dot gmail dot com> <CAMe9rOocmfsT6CiEXqs7cZu-Q+K7kno-ZOFnYkyQAApcSo4zQA at mail dot gmail dot com> <alpine dot LSU dot 2 dot 11 dot 1510071053050 dot 6516 at zhemvz dot fhfr dot qr> <CAFULd4bYhn3wz9u1Gbg0Axo7NNysfc718FjCeQ9ybqQV1+cehA at mail dot gmail dot com>
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.