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 bernd.edlin...@hotmail.de
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 bernd.edlin...@hotmail.de
PR ipa/61190
* g++.old-deja/g++.mike/p4736b.C: Use -O2.
patch-pr61190.diff
Description: Binary data
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 bernd.edlin...@hotmail.de
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 bernd.edlin...@hotmail.de
PR ipa/61190
* g++.old-deja/g++.mike/p4736b.C: Use -O2.
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
bernd.edlin...@hotmail.de 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
On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
bernd.edlin...@hotmail.de 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.
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
bernd.edlin...@hotmail.de 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
Hi,
On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
bernd.edlin...@hotmail.de 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.
