Re: [PATCH] C++ thunk section names
Ping.
On Wed, Aug 6, 2014 at 2:42 PM, Sriraman Tallam tmsri...@google.com wrote:
Hi,
Just wondering if you got a chance to look at this?
Sri
On Tue, Jul 8, 2014 at 10:45 AM, Sriraman Tallam tmsri...@google.com wrote:
On Tue, Jul 8, 2014 at 10:38 AM, Sriraman Tallam tmsri...@google.com wrote:
On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka hubi...@ucw.cz wrote:
Hello,
I apologize for taking so long to get into this patch. I ad busy time
(wedding
and teaching), should be back in regular schedule now.
Sri, can you provide examples to show why putting thunks into the same
section as the target function when function reorder is on can be bad
?
C++ ABI specify that they are in the same section, but I can't think of the
case where this would break.
Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks -
you end up with code in two sections where one is accessing local comdat
of the toher. I would also like to see testcase that breaks and is fixed by
your patch. I would expect that here, by not copying the section name,
you actually make things wose.
Here is an example where the thunk and the original function get
placed in different sections.
class base_class_1
{
public:
virtual void vfn () {}
};
class base_class_2
{
public:
virtual void vfn () {}
};
void foo();
class need_thunk_class : public base_class_1, public base_class_2
{
public:
virtual void vfn () {
for (int i = 0; i 10; ++i)
foo();
}
};
int main (int argc, char *argv[])
{
base_class_1 *bc1 = new need_thunk_class ();
bc1-vfn();
return 0;
}
int glob = 0;
__attribute__((noinline))
void foo()
{
glob++;
}
I am making the function that needs thunk hot. Now,
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition
-fprofile-generate -ffunction-sections
$ a.out
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition -fprofile-use
-ffunction-sections -c
$ objdump -d thunkex.o
Disassembly of section .text.hot._ZN16need_thunk_class3vfnEv:
_ZN16need_thunk_class3vfnEv:
0: 53 push %rbx
1: bb a0 86 01 00 mov$0x186a0,%ebx
...
Disassembly of section .text._ZN16need_thunk_class3vfnEv:
_ZThn8_N16need_thunk_class3vfnEv:
0: 48 83 ef 08 sub$0x8,%rdi
When the original function gets moved to .text.hot, the thunk does
not. It is not always the case that the thunk should either.
I forgot to add that this becomes confusing because, in this case, the
thunk is the only function sitting in a section whose name does not
correspond to its assembler name. If we are not going to have thunk
section names the same as the original function when profiles are
available and -freorder-functions is used, we as well change the name
of the thunk's section to correspond to its assembler name. That was
the intention of the patch.
Thanks
Sri
Thanks
Sri
I think we need to deal with this later; use_tunk is done long before
profiling is read and before we decide whether code is hot/cold. I suppose
the function reordering code may need to always walk whole comdat group and
ensure that sections are same?
I.e. pick the highest profile of a function in the group, resolve unique
section
on it and then copy section names? I had verifier checking that section
names
within one comdat groups are same, perhaps it was part of the reverted
patch
for AIX. I will try to get that one back in now.
Jan
Thanks,
David
On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam tmsri...@google.com
wrote:
Hi Honza,
Could you review this patch when you find time?
Thanks
Sri
On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam
tmsri...@google.com wrote:
Ping.
On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam
tmsri...@google.com wrote:
Ping.
On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam
tmsri...@google.com wrote:
Hi,
I would like this patch reviewed and considered for commit when
Stage 1 is active again.
Patch Description:
A C++ thunk's section name is set to be the same as the original
function's
section name for which the thunk was created in order to place the
two
together. This is done in cp/method.c in function use_thunk.
However, with function reordering turned on, the original
function's section
name can change to something like .text.hot.orginal or
.text.unlikely.original in function default_function_section
in varasm.c
based on the node count of that function. The thunk function's
section name
is not updated to be the same as the original here and also is not
always
correct to do it as the original function can be hotter than the
thunk.
I have created
Re: [PATCH] C++ thunk section names
On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka hubi...@ucw.cz wrote:
Hello,
I apologize for taking so long to get into this patch. I ad busy time
(wedding
and teaching), should be back in regular schedule now.
Sri, can you provide examples to show why putting thunks into the same
section as the target function when function reorder is on can be bad
?
C++ ABI specify that they are in the same section, but I can't think of the
case where this would break.
Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks -
you end up with code in two sections where one is accessing local comdat
of the toher. I would also like to see testcase that breaks and is fixed by
your patch. I would expect that here, by not copying the section name,
you actually make things wose.
Here is an example where the thunk and the original function get
placed in different sections.
class base_class_1
{
public:
virtual void vfn () {}
};
class base_class_2
{
public:
virtual void vfn () {}
};
void foo();
class need_thunk_class : public base_class_1, public base_class_2
{
public:
virtual void vfn () {
for (int i = 0; i 10; ++i)
foo();
}
};
int main (int argc, char *argv[])
{
base_class_1 *bc1 = new need_thunk_class ();
bc1-vfn();
return 0;
}
int glob = 0;
__attribute__((noinline))
void foo()
{
glob++;
}
I am making the function that needs thunk hot. Now,
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition
-fprofile-generate -ffunction-sections
$ a.out
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition -fprofile-use
-ffunction-sections -c
$ objdump -d thunkex.o
Disassembly of section .text.hot._ZN16need_thunk_class3vfnEv:
_ZN16need_thunk_class3vfnEv:
0: 53 push %rbx
1: bb a0 86 01 00 mov$0x186a0,%ebx
...
Disassembly of section .text._ZN16need_thunk_class3vfnEv:
_ZThn8_N16need_thunk_class3vfnEv:
0: 48 83 ef 08 sub$0x8,%rdi
When the original function gets moved to .text.hot, the thunk does
not. It is not always the case that the thunk should either.
Thanks
Sri
I think we need to deal with this later; use_tunk is done long before
profiling is read and before we decide whether code is hot/cold. I suppose
the function reordering code may need to always walk whole comdat group and
ensure that sections are same?
I.e. pick the highest profile of a function in the group, resolve unique
section
on it and then copy section names? I had verifier checking that section names
within one comdat groups are same, perhaps it was part of the reverted patch
for AIX. I will try to get that one back in now.
Jan
Thanks,
David
On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam tmsri...@google.com
wrote:
Hi Honza,
Could you review this patch when you find time?
Thanks
Sri
On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com
wrote:
Hi,
I would like this patch reviewed and considered for commit when
Stage 1 is active again.
Patch Description:
A C++ thunk's section name is set to be the same as the original
function's
section name for which the thunk was created in order to place the two
together. This is done in cp/method.c in function use_thunk.
However, with function reordering turned on, the original function's
section
name can change to something like .text.hot.orginal or
.text.unlikely.original in function default_function_section in
varasm.c
based on the node count of that function. The thunk function's
section name
is not updated to be the same as the original here and also is not
always
correct to do it as the original function can be hotter than the
thunk.
I have created a patch to not name the thunk function's section to be
the same
as the original function when function reordering is enabled.
Thanks
Sri
Re: [PATCH] C++ thunk section names
On Tue, Jul 8, 2014 at 10:38 AM, Sriraman Tallam tmsri...@google.com wrote:
On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka hubi...@ucw.cz wrote:
Hello,
I apologize for taking so long to get into this patch. I ad busy time
(wedding
and teaching), should be back in regular schedule now.
Sri, can you provide examples to show why putting thunks into the same
section as the target function when function reorder is on can be bad
?
C++ ABI specify that they are in the same section, but I can't think of the
case where this would break.
Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks -
you end up with code in two sections where one is accessing local comdat
of the toher. I would also like to see testcase that breaks and is fixed by
your patch. I would expect that here, by not copying the section name,
you actually make things wose.
Here is an example where the thunk and the original function get
placed in different sections.
class base_class_1
{
public:
virtual void vfn () {}
};
class base_class_2
{
public:
virtual void vfn () {}
};
void foo();
class need_thunk_class : public base_class_1, public base_class_2
{
public:
virtual void vfn () {
for (int i = 0; i 10; ++i)
foo();
}
};
int main (int argc, char *argv[])
{
base_class_1 *bc1 = new need_thunk_class ();
bc1-vfn();
return 0;
}
int glob = 0;
__attribute__((noinline))
void foo()
{
glob++;
}
I am making the function that needs thunk hot. Now,
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition
-fprofile-generate -ffunction-sections
$ a.out
$ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition -fprofile-use
-ffunction-sections -c
$ objdump -d thunkex.o
Disassembly of section .text.hot._ZN16need_thunk_class3vfnEv:
_ZN16need_thunk_class3vfnEv:
0: 53 push %rbx
1: bb a0 86 01 00 mov$0x186a0,%ebx
...
Disassembly of section .text._ZN16need_thunk_class3vfnEv:
_ZThn8_N16need_thunk_class3vfnEv:
0: 48 83 ef 08 sub$0x8,%rdi
When the original function gets moved to .text.hot, the thunk does
not. It is not always the case that the thunk should either.
I forgot to add that this becomes confusing because, in this case, the
thunk is the only function sitting in a section whose name does not
correspond to its assembler name. If we are not going to have thunk
section names the same as the original function when profiles are
available and -freorder-functions is used, we as well change the name
of the thunk's section to correspond to its assembler name. That was
the intention of the patch.
Thanks
Sri
Thanks
Sri
I think we need to deal with this later; use_tunk is done long before
profiling is read and before we decide whether code is hot/cold. I suppose
the function reordering code may need to always walk whole comdat group and
ensure that sections are same?
I.e. pick the highest profile of a function in the group, resolve unique
section
on it and then copy section names? I had verifier checking that section
names
within one comdat groups are same, perhaps it was part of the reverted patch
for AIX. I will try to get that one back in now.
Jan
Thanks,
David
On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam tmsri...@google.com
wrote:
Hi Honza,
Could you review this patch when you find time?
Thanks
Sri
On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com
wrote:
Ping.
On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam
tmsri...@google.com wrote:
Ping.
On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com
wrote:
Hi,
I would like this patch reviewed and considered for commit when
Stage 1 is active again.
Patch Description:
A C++ thunk's section name is set to be the same as the original
function's
section name for which the thunk was created in order to place the
two
together. This is done in cp/method.c in function use_thunk.
However, with function reordering turned on, the original function's
section
name can change to something like .text.hot.orginal or
.text.unlikely.original in function default_function_section in
varasm.c
based on the node count of that function. The thunk function's
section name
is not updated to be the same as the original here and also is not
always
correct to do it as the original function can be hotter than the
thunk.
I have created a patch to not name the thunk function's section to
be the same
as the original function when function reordering is enabled.
Thanks
Sri
Re: [PATCH] C++ thunk section names
Sri, can you provide examples to show why putting thunks into the same section as the target function when function reorder is on can be bad ? Thanks, David On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam tmsri...@google.com wrote: Hi Honza, Could you review this patch when you find time? Thanks Sri On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Hello, I apologize for taking so long to get into this patch. I ad busy time (wedding and teaching), should be back in regular schedule now. Sri, can you provide examples to show why putting thunks into the same section as the target function when function reorder is on can be bad ? C++ ABI specify that they are in the same section, but I can't think of the case where this would break. Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks - you end up with code in two sections where one is accessing local comdat of the toher. I would also like to see testcase that breaks and is fixed by your patch. I would expect that here, by not copying the section name, you actually make things wose. I think we need to deal with this later; use_tunk is done long before profiling is read and before we decide whether code is hot/cold. I suppose the function reordering code may need to always walk whole comdat group and ensure that sections are same? I.e. pick the highest profile of a function in the group, resolve unique section on it and then copy section names? I had verifier checking that section names within one comdat groups are same, perhaps it was part of the reverted patch for AIX. I will try to get that one back in now. Jan Thanks, David On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam tmsri...@google.com wrote: Hi Honza, Could you review this patch when you find time? Thanks Sri On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Hi Honza, Could you review this patch when you find time? Thanks Sri On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Ping. On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Ping. On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Ping. On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam tmsri...@google.com wrote: Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
Re: [PATCH] C++ thunk section names
Ping. On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam tmsri...@google.com wrote: Hi, I would like this patch reviewed and considered for commit when Stage 1 is active again. Patch Description: A C++ thunk's section name is set to be the same as the original function's section name for which the thunk was created in order to place the two together. This is done in cp/method.c in function use_thunk. However, with function reordering turned on, the original function's section name can change to something like .text.hot.orginal or .text.unlikely.original in function default_function_section in varasm.c based on the node count of that function. The thunk function's section name is not updated to be the same as the original here and also is not always correct to do it as the original function can be hotter than the thunk. I have created a patch to not name the thunk function's section to be the same as the original function when function reordering is enabled. Thanks Sri
