Re: [PATCH] Fix PR ipa/61190, updated
> > Index: gcc/ipa-pure-const.c
> > ===
> > --- gcc/ipa-pure-const.c (revision 215888)
> > +++ gcc/ipa-pure-const.c (working copy)
> > @@ -744,6 +744,8 @@ analyze_function (struct cgraph_node *fn, bool ipa
> > {
> > /* Thunk gets propagated through, so nothing interesting happens. */
> > gcc_assert (ipa);
> > + if (fn->thunk.virtual_offset_p)
> > + l->pure_const_state = IPA_NEITHER;
> > return l;
> > }
> >
>
> Hmm, I looked again at the above if statement, and I think now it should
> better be "if (fn->thunk.thunk_p && fn->thunk.virtual_offset_p)", because
> thunk.virtual_offset_p is probably not well defined if we come here because
> of fn->alias == true.
Yes, that is right. I plan to put the other thunk info off the structure
anyway.
>
> > This makes the lattice to be initialized correctly, but you also need the
> > function_symbol calls that will skip thunks replaced by
> > something like function_or_non_virtual_thunk_symbol.
> >
>
> Oh, I see what you mean, thanks.
>
> I created a new method function_or_virtual_thunk_symbol() for this.
> And simplified the algorithm of both function_symbol variants a bit.
>
> Attached, you'll find my updated patch for review.
>
> Boot-strapped and regression tested on x86_64-linux-gnu.
> OK for trunk?
>
>
> Thanks
> Bernd.
>
> > Can you, please, send the updated patch?
> > Sorry for late review,
> > Honza
> >
>
>
> 2014-11-25 Bernd Edlinger
>
> PR ipa/61190
> * cgraph.h (symtab_node::call_for_symbol_and_aliases): Fix comment.
> (cgraph_node::function_or_virtual_thunk_symbol): New function.
> (cgraph_node::call_for_symbol_and_aliases): Fix comment.
> (cgraph_node::call_for_symbol_thunks_and_aliases): Adjust comment.
> Add new optional parameter exclude_virtual_thunks.
> * cgraph.c (cgraph_node::call_for_symbol_thunks_and_aliases): Add new
> optional parameter exclude_virtual_thunks.
> (cgraph_node::set_const_flag): Don't propagate to virtual thunks.
> (cgraph_node::set_pure_flag): Likewise.
> (cgraph_node::function_symbol): Simplified.
> (cgraph_node::function_or_virtual_thunk_symbol): New function.
> * ipa-pure-const.c (analyze_function): For virtual thunks set
> pure_const_state to IPA_NEITHER.
> (propagate_pure_const): Use function_or_virtual_thunk_symbol.
OK,
Honza
>
> testsuite/ChangeLog:
> 2014-11-25 Bernd Edlinger
>
> PR ipa/61190
> * g++.old-deja/g++.mike/p4736b.C: Use -O2.
>
RE: [PATCH] Fix PR ipa/61190, updated
Hi Honza,
On Mon, 24 Nov 2014 16:57:42 +0100, Jan Hubicka wrote:
>
> +cgraph_node::call_for_symbol_thunks_and_aliases_1 (bool (*callback)
> + (cgraph_node *, void *),
> + void *data,
> + bool include_overwritable,
> + bool exclude_virtual_thunks)
>
> Instead of adding _1 variant into public API, please just add implicit
> agrumnet
> bool exclude_virtual_thunks=false into
> +cgraph_node::call_for_symbol_thunks_and_aliases
Ok, done.
> Index: gcc/ipa-pure-const.c
> ===
> --- gcc/ipa-pure-const.c (revision 215888)
> +++ gcc/ipa-pure-const.c (working copy)
> @@ -744,6 +744,8 @@ analyze_function (struct cgraph_node *fn, bool ipa
> {
> /* Thunk gets propagated through, so nothing interesting happens. */
> gcc_assert (ipa);
> + if (fn->thunk.virtual_offset_p)
> + l->pure_const_state = IPA_NEITHER;
> return l;
> }
>
Hmm, I looked again at the above if statement, and I think now it should
better be "if (fn->thunk.thunk_p && fn->thunk.virtual_offset_p)", because
thunk.virtual_offset_p is probably not well defined if we come here because
of fn->alias == true.
> This makes the lattice to be initialized correctly, but you also need the
> function_symbol calls that will skip thunks replaced by
> something like function_or_non_virtual_thunk_symbol.
>
Oh, I see what you mean, thanks.
I created a new method function_or_virtual_thunk_symbol() for this.
And simplified the algorithm of both function_symbol variants a bit.
Attached, you'll find my updated patch for review.
Boot-strapped and regression tested on x86_64-linux-gnu.
OK for trunk?
Thanks
Bernd.
> Can you, please, send the updated patch?
> Sorry for late review,
> Honza
>
2014-11-25 Bernd Edlinger
PR ipa/61190
* cgraph.h (symtab_node::call_for_symbol_and_aliases): Fix comment.
(cgraph_node::function_or_virtual_thunk_symbol): New function.
(cgraph_node::call_for_symbol_and_aliases): Fix comment.
(cgraph_node::call_for_symbol_thunks_and_aliases): Adjust comment.
Add new optional parameter exclude_virtual_thunks.
* cgraph.c (cgraph_node::call_for_symbol_thunks_and_aliases): Add new
optional parameter exclude_virtual_thunks.
(cgraph_node::set_const_flag): Don't propagate to virtual thunks.
(cgraph_node::set_pure_flag): Likewise.
(cgraph_node::function_symbol): Simplified.
(cgraph_node::function_or_virtual_thunk_symbol): New function.
* ipa-pure-const.c (analyze_function): For virtual thunks set
pure_const_state to IPA_NEITHER.
(propagate_pure_const): Use function_or_virtual_thunk_symbol.
testsuite/ChangeLog:
2014-11-25 Bernd Edlinger
PR ipa/61190
* g++.old-deja/g++.mike/p4736b.C: Use -O2.
patch-pr61190.diff
Description: Binary data
Re: [PING**4] [PATCH] Fix PR ipa/61190, 2nd edition�??
+cgraph_node::call_for_symbol_thunks_and_aliases_1 (bool (*callback)
+(cgraph_node *, void *),
+ void *data,
+ bool include_overwritable,
+ bool exclude_virtual_thunks)
Instead of adding _1 variant into public API, please just add implicit agrumnet
bool exclude_virtual_thunks=false into
+cgraph_node::call_for_symbol_thunks_and_aliases
Index: gcc/ipa-pure-const.c
===
--- gcc/ipa-pure-const.c(revision 215888)
+++ gcc/ipa-pure-const.c(working copy)
@@ -744,6 +744,8 @@ analyze_function (struct cgraph_node *fn, bool ipa
{
/* Thunk gets propagated through, so nothing interesting happens. */
gcc_assert (ipa);
+ if (fn->thunk.virtual_offset_p)
+ l->pure_const_state = IPA_NEITHER;
return l;
}
This makes the lattice to be initialized correctly, but you also need the
function_symbol calls that will skip thunks replaced by
something like function_or_non_virtual_thunk_symbol.
Can you, please, send the updated patch?
Sorry for late review,
Honza
Index: gcc/testsuite/g++.old-deja/g++.mike/p4736b.C
===
--- gcc/testsuite/g++.old-deja/g++.mike/p4736b.C(revision 215888)
+++ gcc/testsuite/g++.old-deja/g++.mike/p4736b.C(working copy)
@@ -1,4 +1,5 @@
// { dg-do run }
+// { dg-options "-O2" }
// prms-id: 4736
class Rep {
RE: [PING**4] [PATCH] Fix PR ipa/61190, 2nd editionâ
Hello everybody, currently this makes the virtual thunks and the virtual C++ base classes pretty much broken. So it would be really nice if some one would like to review this patch. I attached the patch again, for your convenience. Thanks Bernd. > From: bernd.edlin...@hotmail.de > To: gcc-patches@gcc.gnu.org > CC: richard.guent...@gmail.com; hubi...@ucw.cz > Subject: RE: [PING**3] [PATCH] Fix PR ipa/61190, 2nd editionâ > Date: Wed, 12 Nov 2014 13:43:18 +0100 > > > > Ping... > > > >> From: bernd.edlin...@hotmail.de >> To: gcc-patches@gcc.gnu.org >> CC: richard.guent...@gmail.com; hubi...@ucw.cz >> Subject: RE: [PING**2] [PATCH] Fix PR ipa/61190, 2nd editionâ >> Date: Mon, 27 Oct 2014 09:23:41 +0100 >> >> >> Ping again... >> >> Thanks. >> >> >>> From: bernd.edlin...@hotmail.de >>> To: hubi...@ucw.cz >>> CC: gcc-patches@gcc.gnu.org; richard.guent...@gmail.com >>> Subject: [PING] [PATCH] Fix PR ipa/61190, 2nd editionâ >>> Date: Tue, 14 Oct 2014 11:40:56 +0200 >>> >>> Ping... >>> >>> see: https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00536.html >>> >>>> Hi Honza, >>>> >>>> >>>> as you know, we have a wrong code bug, when a pure or const method is >>>> called via a virtual thunk. >>>> I had some more Ideas, how to fix that, but all of them had some serious >>>> draw-backs, so I leave the details out... >>>> >>>> >>>> But now I have a new insight, why the "obvious" fix for this serious code >>>> generation bug did not work >>>> in the first place. >>>> >>>> >>>> And the reason was, that if ipa-pure-const.c calls set_const_flag or >>>> set_pure_flag for a thunk, it calls the same >>>> function later for the called method, and this overwrites the flags of >>>> _all_ associated thunks and aliases. >>>> However that should at least not be done for virtual thunks, as these need >>>> to be IPA_NEITHER, even if >>>> the method itself has different attributes, that is because the assembler >>>> thunk accesses the vtable, while >>>> other thunks do not. >>>> >>>> >>>> So I re-factored set_const_flag and set_pure_flag to exclude the virtual >>>> thunks, taking care that other >>>> users of call_for_symbol_thunks_and_aliases do not get a different >>>> behavior than before this patch. >>>> >>>> >>>> The attached patch was boot-strapped and >>>> regression-tested on x86_64-linux-gnu. >>>> Ok for trunk? >>>> >>>> >>>> PS: As a side-note, there are two identical functions, named >>>> "call_for_symbol_and_aliases", in >>>> class symtab_node and in class cgraph_node, which inherits from >>>> symtab_node. Both functions are >>>> not declared virtual. Is that what's intended? Usually this could lead to >>>> errors, or at least some serious >>>> compiler warnings. >>>> >>>> >>>> Thanks >>>> Bernd. >>>> >>> >> > 2014-10-07 Bernd Edlinger PR ipa/61190 * cgraph.h (symtab_node::call_for_symbol_and_aliases): Fix comment. (cgraph_node::call_for_symbol_and_aliases): Likewise. (cgraph_node::call_for_symbol_thunks_and_aliases_1): New function. (cgraph_node::call_for_symbol_thunks_and_aliases): Adjust comment. Call call_for_symbol_thunks_and_aliases_1. * cgraph.c (cgraph_node::call_for_symbol_thunks_and_aliases): Renamed to cgraph_node::call_for_symbol_thunks_and_aliases_1. Added new parameter exclude_virtual_thunks. (cgraph_node::set_const_flag): Don't propagate to virtual thunks. (cgraph_node::set_pure_flag): Likewise. * ipa-pure-const.c (analyze_function): For virtual thunks set pure_const_state to IPA_NEITHER. testsuite/ChangeLog: 2014-10-07 Bernd Edlinger PR ipa/61190 * g++.old-deja/g++.mike/p4736b.C: Use -O2. patch-pr61190.diff Description: Binary data
RE: [PING**3] [PATCH] Fix PR ipa/61190, 2nd editionâ
Ping... > From: bernd.edlin...@hotmail.de > To: gcc-patches@gcc.gnu.org > CC: richard.guent...@gmail.com; hubi...@ucw.cz > Subject: RE: [PING**2] [PATCH] Fix PR ipa/61190, 2nd editionâ > Date: Mon, 27 Oct 2014 09:23:41 +0100 > > > Ping again... > > Thanks. > > >> From: bernd.edlin...@hotmail.de >> To: hubi...@ucw.cz >> CC: gcc-patches@gcc.gnu.org; richard.guent...@gmail.com >> Subject: [PING] [PATCH] Fix PR ipa/61190, 2nd editionâ >> Date: Tue, 14 Oct 2014 11:40:56 +0200 >> >> Ping... >> >> see: https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00536.html >> >>> Hi Honza, >>> >>> >>> as you know, we have a wrong code bug, when a pure or const method is >>> called via a virtual thunk. >>> I had some more Ideas, how to fix that, but all of them had some serious >>> draw-backs, so I leave the details out... >>> >>> >>> But now I have a new insight, why the "obvious" fix for this serious code >>> generation bug did not work >>> in the first place. >>> >>> >>> And the reason was, that if ipa-pure-const.c calls set_const_flag or >>> set_pure_flag for a thunk, it calls the same >>> function later for the called method, and this overwrites the flags of >>> _all_ associated thunks and aliases. >>> However that should at least not be done for virtual thunks, as these need >>> to be IPA_NEITHER, even if >>> the method itself has different attributes, that is because the assembler >>> thunk accesses the vtable, while >>> other thunks do not. >>> >>> >>> So I re-factored set_const_flag and set_pure_flag to exclude the virtual >>> thunks, taking care that other >>> users of call_for_symbol_thunks_and_aliases do not get a different behavior >>> than before this patch. >>> >>> >>> The attached patch was boot-strapped and >>> regression-tested on x86_64-linux-gnu. >>> Ok for trunk? >>> >>> >>> PS: As a side-note, there are two identical functions, named >>> "call_for_symbol_and_aliases", in >>> class symtab_node and in class cgraph_node, which inherits from >>> symtab_node. Both functions are >>> not declared virtual. Is that what's intended? Usually this could lead to >>> errors, or at least some serious >>> compiler warnings. >>> >>> >>> Thanks >>> Bernd. >>> >> >
RE: [PING**2] [PATCH] Fix PR ipa/61190, 2nd edition
Ping again... Thanks. > From: bernd.edlin...@hotmail.de > To: hubi...@ucw.cz > CC: gcc-patches@gcc.gnu.org; richard.guent...@gmail.com > Subject: [PING] [PATCH] Fix PR ipa/61190, 2nd edition > Date: Tue, 14 Oct 2014 11:40:56 +0200 > > Ping... > > see: https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00536.html > >> Hi Honza, >> >> >> as you know, we have a wrong code bug, when a pure or const method is called >> via a virtual thunk. >> I had some more Ideas, how to fix that, but all of them had some serious >> draw-backs, so I leave the details out... >> >> >> But now I have a new insight, why the "obvious" fix for this serious code >> generation bug did not work >> in the first place. >> >> >> And the reason was, that if ipa-pure-const.c calls set_const_flag or >> set_pure_flag for a thunk, it calls the same >> function later for the called method, and this overwrites the flags of _all_ >> associated thunks and aliases. >> However that should at least not be done for virtual thunks, as these need >> to be IPA_NEITHER, even if >> the method itself has different attributes, that is because the assembler >> thunk accesses the vtable, while >> other thunks do not. >> >> >> So I re-factored set_const_flag and set_pure_flag to exclude the virtual >> thunks, taking care that other >> users of call_for_symbol_thunks_and_aliases do not get a different behavior >> than before this patch. >> >> >> The attached patch was boot-strapped and >> regression-tested on x86_64-linux-gnu. >> Ok for trunk? >> >> >> PS: As a side-note, there are two identical functions, named >> "call_for_symbol_and_aliases", in >> class symtab_node and in class cgraph_node, which inherits from symtab_node. >> Both functions are >> not declared virtual. Is that what's intended? Usually this could lead to >> errors, or at least some serious >> compiler warnings. >> >> >> Thanks >> Bernd. >> >
[PING] [PATCH] Fix PR ipa/61190, 2nd edition
Ping... see: https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00536.html > Hi Honza, > > > as you know, we have a wrong code bug, when a pure or const method is called > via a virtual thunk. > I had some more Ideas, how to fix that, but all of them had some serious > draw-backs, so I leave the details out... > > > But now I have a new insight, why the "obvious" fix for this serious code > generation bug did not work > in the first place. > > > And the reason was, that if ipa-pure-const.c calls set_const_flag or > set_pure_flag for a thunk, it calls the same > function later for the called method, and this overwrites the flags of _all_ > associated thunks and aliases. > However that should at least not be done for virtual thunks, as these need to > be IPA_NEITHER, even if > the method itself has different attributes, that is because the assembler > thunk accesses the vtable, while > other thunks do not. > > > So I re-factored set_const_flag and set_pure_flag to exclude the virtual > thunks, taking care that other > users of call_for_symbol_thunks_and_aliases do not get a different behavior > than before this patch. > > > The attached patch was boot-strapped and > regression-tested on x86_64-linux-gnu. > Ok for trunk? > > > PS: As a side-note, there are two identical functions, named > "call_for_symbol_and_aliases", in > class symtab_node and in class cgraph_node, which inherits from symtab_node. > Both functions are > not declared virtual. Is that what's intended? Usually this could lead to > errors, or at least some serious > compiler warnings. > > > Thanks > Bernd. >
[PATCH] Fix PR ipa/61190, 2nd edition
Hi Honza, as you know, we have a wrong code bug, when a pure or const method is called via a virtual thunk. I had some more Ideas, how to fix that, but all of them had some serious draw-backs, so I leave the details out... But now I have a new insight, why the "obvious" fix for this serious code generation bug did not work in the first place. And the reason was, that if ipa-pure-const.c calls set_const_flag or set_pure_flag for a thunk, it calls the same function later for the called method, and this overwrites the flags of _all_ associated thunks and aliases. However that should at least not be done for virtual thunks, as these need to be IPA_NEITHER, even if the method itself has different attributes, that is because the assembler thunk accesses the vtable, while other thunks do not. So I re-factored set_const_flag and set_pure_flag to exclude the virtual thunks, taking care that other users of call_for_symbol_thunks_and_aliases do not get a different behavior than before this patch. The attached patch was boot-strapped and regression-tested on x86_64-linux-gnu. Ok for trunk? PS: As a side-note, there are two identical functions, named "call_for_symbol_and_aliases", in class symtab_node and in class cgraph_node, which inherits from symtab_node. Both functions are not declared virtual. Is that what's intended? Usually this could lead to errors, or at least some serious compiler warnings. Thanks Bernd. 2014-10-07 Bernd Edlinger PR ipa/61190 * cgraph.h (symtab_node::call_for_symbol_and_aliases): Fix comment. (cgraph_node::call_for_symbol_and_aliases): Likewise. (cgraph_node::call_for_symbol_thunks_and_aliases_1): New function. (cgraph_node::call_for_symbol_thunks_and_aliases): Adjust comment. Call call_for_symbol_thunks_and_aliases_1. * cgraph.c (cgraph_node::call_for_symbol_thunks_and_aliases): Renamed to cgraph_node::call_for_symbol_thunks_and_aliases_1. Added new parameter exclude_virtual_thunks. (cgraph_node::set_const_flag): Don't propagate to virtual thunks. (cgraph_node::set_pure_flag): Likewise. * ipa-pure-const.c (analyze_function): For virtual thunks set pure_const_state to IPA_NEITHER. testsuite/ChangeLog: 2014-10-07 Bernd Edlinger PR ipa/61190 * g++.old-deja/g++.mike/p4736b.C: Use -O2. patch-pr61190.diff Description: Binary data
RE: [PATCH] Fix PR ipa/61190
Hi Honza,
On Mon, 2 Jun 2014 18:12:10, Jan Hubicka wrote:
>
>> Hi,
>>
>> On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
>>>
>>> On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
>>> wrote:
Hi,
the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
optimization levels, except -O0 and -Og. This probably started with gcc
4.7.x.
The constructor Main::Main() is first inlined and then completely optimized
away in the dce1 pass. But the thunk is still using the vtable, and
therefore
crashes.
>
> Why the ctor is eliminated? Is it because ipa-pure-const identifies the thunk
> as const?
> I think we need to update it to notice the read of vtable in those thunks. I
> will
> take a look.
>
have you been able to find an alternative solution yet?
Thanks
Bernd.
> Honza
Well, the problem seems to be, that the thunk code is
not emitted in the normal way using gimple code,
but instead by the i386 back-end with a callback.
This makes the thunk invisible to the ipa-pure-const pass,
which assumes that all values just flow straight thu the thunk.
See ipa-pure-const.c (analyze_function):
if (fn->thunk.thunk_p || fn->alias)
{
/* Thunk gets propagated through, so nothing interesting happens. */
gcc_assert (ipa);
return l;
}
But this is not true for a virtual base class:
The thunk itself references the vtable, so if nothing else might need
the vtable, the optimizer may remove the initalization of the vtable,
which happened in this example.
The most simple work around would be the attached patch, which
simply falls back to emitting gimple code, if virtual base classes
are used.
Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
Ok for trunk?
>>>
>>> Honza should know better. I'd rather skip the above for
>>> thunks going the output_mi_thunk way.
>>>
>>
>> Ahh Yes, that was of course my first attempt...
>>
>> But there is not BB to enumerate in that case.
>> And the loop below just crashed in:
>>
>> FOR_EACH_BB_FN (this_block, cfun)
>>
>>
>> There is also another interesting thing to mention: when the
>> output_mi_thunk outputs the virtual thunk, there is no inlining at all.
>>
>> But with this patch, the thunk inlines the called function,
>> and therefore the generated code looks rather nifty, compared to
>> the state before the patch.
>>
>>
>> Thanks
>> Bernd.
>>
>>> That is, the target hook docs should be updated to reflect which
>>> kind of thunks it is supposed to handle.
>>>
>>> Richard.
>>>
Thanks
Bernd.
>>
Re: [PATCH] Fix PR ipa/61190
> Hi,
>
> On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
> >
> > On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
> > wrote:
> >> Hi,
> >>
> >> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
> >> optimization levels, except -O0 and -Og. This probably started with gcc
> >> 4.7.x.
> >>
> >> The constructor Main::Main() is first inlined and then completely optimized
> >> away in the dce1 pass. But the thunk is still using the vtable, and
> >> therefore
> >> crashes.
Why the ctor is eliminated? Is it because ipa-pure-const identifies the thunk
as const?
I think we need to update it to notice the read of vtable in those thunks. I
will
take a look.
Honza
> >>
> >> Well, the problem seems to be, that the thunk code is
> >> not emitted in the normal way using gimple code,
> >> but instead by the i386 back-end with a callback.
> >>
> >> This makes the thunk invisible to the ipa-pure-const pass,
> >> which assumes that all values just flow straight thu the thunk.
> >>
> >> See ipa-pure-const.c (analyze_function):
> >>
> >> if (fn->thunk.thunk_p || fn->alias)
> >> {
> >> /* Thunk gets propagated through, so nothing interesting happens. */
> >> gcc_assert (ipa);
> >> return l;
> >> }
> >>
> >> But this is not true for a virtual base class:
> >> The thunk itself references the vtable, so if nothing else might need
> >> the vtable, the optimizer may remove the initalization of the vtable,
> >> which happened in this example.
> >>
> >> The most simple work around would be the attached patch, which
> >> simply falls back to emitting gimple code, if virtual base classes
> >> are used.
> >>
> >> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
> >> Ok for trunk?
> >
> > Honza should know better. I'd rather skip the above for
> > thunks going the output_mi_thunk way.
> >
>
> Ahh Yes, that was of course my first attempt...
>
> But there is not BB to enumerate in that case.
> And the loop below just crashed in:
>
> FOR_EACH_BB_FN (this_block, cfun)
>
>
> There is also another interesting thing to mention: when the
> output_mi_thunk outputs the virtual thunk, there is no inlining at all.
>
> But with this patch, the thunk inlines the called function,
> and therefore the generated code looks rather nifty, compared to
> the state before the patch.
>
>
> Thanks
> Bernd.
>
> > That is, the target hook docs should be updated to reflect which
> > kind of thunks it is supposed to handle.
> >
> > Richard.
> >
> >>
> >> Thanks
> >> Bernd.
> >>
>
RE: [PATCH] Fix PR ipa/61190
Hi,
On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
>
> On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
> wrote:
>> Hi,
>>
>> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
>> optimization levels, except -O0 and -Og. This probably started with gcc
>> 4.7.x.
>>
>> The constructor Main::Main() is first inlined and then completely optimized
>> away in the dce1 pass. But the thunk is still using the vtable, and therefore
>> crashes.
>>
>> Well, the problem seems to be, that the thunk code is
>> not emitted in the normal way using gimple code,
>> but instead by the i386 back-end with a callback.
>>
>> This makes the thunk invisible to the ipa-pure-const pass,
>> which assumes that all values just flow straight thu the thunk.
>>
>> See ipa-pure-const.c (analyze_function):
>>
>> if (fn->thunk.thunk_p || fn->alias)
>> {
>> /* Thunk gets propagated through, so nothing interesting happens. */
>> gcc_assert (ipa);
>> return l;
>> }
>>
>> But this is not true for a virtual base class:
>> The thunk itself references the vtable, so if nothing else might need
>> the vtable, the optimizer may remove the initalization of the vtable,
>> which happened in this example.
>>
>> The most simple work around would be the attached patch, which
>> simply falls back to emitting gimple code, if virtual base classes
>> are used.
>>
>> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
>> Ok for trunk?
>
> Honza should know better. I'd rather skip the above for
> thunks going the output_mi_thunk way.
>
Ahh Yes, that was of course my first attempt...
But there is not BB to enumerate in that case.
And the loop below just crashed in:
FOR_EACH_BB_FN (this_block, cfun)
There is also another interesting thing to mention: when the
output_mi_thunk outputs the virtual thunk, there is no inlining at all.
But with this patch, the thunk inlines the called function,
and therefore the generated code looks rather nifty, compared to
the state before the patch.
Thanks
Bernd.
> That is, the target hook docs should be updated to reflect which
> kind of thunks it is supposed to handle.
>
> Richard.
>
>>
>> Thanks
>> Bernd.
>>
Re: [PATCH] Fix PR ipa/61190
On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
wrote:
> Hi,
>
> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
> optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
>
> The constructor Main::Main() is first inlined and then completely optimized
> away in the dce1 pass. But the thunk is still using the vtable, and therefore
> crashes.
>
> Well, the problem seems to be, that the thunk code is
> not emitted in the normal way using gimple code,
> but instead by the i386 back-end with a callback.
>
> This makes the thunk invisible to the ipa-pure-const pass,
> which assumes that all values just flow straight thu the thunk.
>
> See ipa-pure-const.c (analyze_function):
>
> if (fn->thunk.thunk_p || fn->alias)
> {
> /* Thunk gets propagated through, so nothing interesting happens. */
> gcc_assert (ipa);
> return l;
> }
>
> But this is not true for a virtual base class:
> The thunk itself references the vtable, so if nothing else might need
> the vtable, the optimizer may remove the initalization of the vtable,
> which happened in this example.
>
> The most simple work around would be the attached patch, which
> simply falls back to emitting gimple code, if virtual base classes
> are used.
>
> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
> Ok for trunk?
Honza should know better. I'd rather skip the above for
thunks going the output_mi_thunk way.
That is, the target hook docs should be updated to reflect which
kind of thunks it is supposed to handle.
Richard.
>
> Thanks
> Bernd.
>
[PATCH] Fix PR ipa/61190
Hi,
the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
The constructor Main::Main() is first inlined and then completely optimized
away in the dce1 pass. But the thunk is still using the vtable, and therefore
crashes.
Well, the problem seems to be, that the thunk code is
not emitted in the normal way using gimple code,
but instead by the i386 back-end with a callback.
This makes the thunk invisible to the ipa-pure-const pass,
which assumes that all values just flow straight thu the thunk.
See ipa-pure-const.c (analyze_function):
if (fn->thunk.thunk_p || fn->alias)
{
/* Thunk gets propagated through, so nothing interesting happens. */
gcc_assert (ipa);
return l;
}
But this is not true for a virtual base class:
The thunk itself references the vtable, so if nothing else might need
the vtable, the optimizer may remove the initalization of the vtable,
which happened in this example.
The most simple work around would be the attached patch, which
simply falls back to emitting gimple code, if virtual base classes
are used.
Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
Ok for trunk?
Thanks
Bernd.
2014-06-02 Bernd Edlinger
PR ipa/61190
* cgraphunit.c (expand_thunk): Don't try to output the thunk for
a virtual base class via targetm.asm_out.output_mi_thunk.
testsuite/ChangeLog:
2014-06-02 Bernd Edlinger
PR ipa/61190
* g++.old-deja/g++.mike/p4736b.C: Use -O2.
patch-pr61190.diff
Description: Binary data
