diff mbox

PR middle-end/51411: handle transaction_safe virtual inlined methods

Message ID 4EE7BA39.6050900@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Dec. 13, 2011, 8:48 p.m. UTC 
On 12/13/11 14:42, Richard Henderson wrote:
> On 12/13/2011 12:41 PM, Aldy Hernandez wrote:
>> The TM clone is marked weak as the original decl was weak, but it is no longer external or public.  I suggested to just clear the DECL_WEAK bit on the clone, but Jason mentioned that unsetting DECL_EXTERNAL is more optimal-- so that the clone can still be public and comdat.
>>
>> No regressions.  PR fixed.
>>
>> OK?
>>
>> curr
>>
>>
>> 	PR middle-end/51411
>> 	* trans-mem.c (ipa_tm_create_version): Do not zap DECL_EXTERNAL.
>
> The actual patch is more or less the exact opposite of what Jason recommended.
>
>
> r~

Bah, you guys are speaking Bostoneese or something, cause reading back 
on the IRC log I still think I implemented what Jason said.

Be that as it may, I gather this is what you want?

OK pending another round of tests?
PR middle-end/51411
	* trans-mem.c (ipa_tm_create_version): Do not zap DECL_EXTERNAL.

Comments

Jason Merrill Dec. 13, 2011, 8:52 p.m. UTC | #1 
On 12/13/2011 03:48 PM, Aldy Hernandez wrote:
> Be that as it may, I gather this is what you want?

Yep, that's what I had in mind.

Jason
Richard Henderson Dec. 13, 2011, 9:02 p.m. UTC | #2 
On 12/13/2011 12:48 PM, Aldy Hernandez wrote:
> 	PR middle-end/51411
> 	* trans-mem.c (ipa_tm_create_version): Do not zap DECL_EXTERNAL.
...
>        /* ??? Is it worth trying to use make_decl_one_only?  */
>        if (DECL_DECLARED_INLINE_P (new_decl) && DECL_EXTERNAL (new_decl))
> -	{
> -	  DECL_EXTERNAL (new_decl) = 0;
> -	  TREE_PUBLIC (new_decl) = 0;
> -	}
> +	DECL_EXTERNAL (new_decl) = 0;

Yes, that's what we had in mind.  Though of course the changelog doesn't match.


r~
Aldy Hernandez Dec. 13, 2011, 9:04 p.m. UTC | #3 
On 12/13/11 15:02, Richard Henderson wrote:
> On 12/13/2011 12:48 PM, Aldy Hernandez wrote:
>> 	PR middle-end/51411
>> 	* trans-mem.c (ipa_tm_create_version): Do not zap DECL_EXTERNAL.
> ...
>>         /* ??? Is it worth trying to use make_decl_one_only?  */
>>         if (DECL_DECLARED_INLINE_P (new_decl)&&  DECL_EXTERNAL (new_decl))
>> -	{
>> -	  DECL_EXTERNAL (new_decl) = 0;
>> -	  TREE_PUBLIC (new_decl) = 0;
>> -	}
>> +	DECL_EXTERNAL (new_decl) = 0;
>
> Yes, that's what we had in mind.  Though of course the changelog doesn't match.
>
>
> r~

Ok, my English typing privileges have been revoked until further notice. 
  I'm obviously typing cross-eyed right now.

Here's what I have, but keep in mind that I'm going to watch Sesame 
Street so I'll be away from my keyboard for a few hours...

         PR middle-end/51411
         * trans-mem.c (ipa_tm_create_version): Do not zap TREE_PUBLIC.
Patrick Marlier Dec. 14, 2011, 12:20 a.m. UTC | #4 
On 12/13/2011 04:04 PM, Aldy Hernandez wrote:
> On 12/13/11 15:02, Richard Henderson wrote:
>> On 12/13/2011 12:48 PM, Aldy Hernandez wrote:
>>> PR middle-end/51411
>>> * trans-mem.c (ipa_tm_create_version): Do not zap DECL_EXTERNAL.
>> ...
>>> /* ??? Is it worth trying to use make_decl_one_only? */
>>> if (DECL_DECLARED_INLINE_P (new_decl)&& DECL_EXTERNAL (new_decl))
>>> - {
>>> - DECL_EXTERNAL (new_decl) = 0;
>>> - TREE_PUBLIC (new_decl) = 0;
>>> - }
>>> + DECL_EXTERNAL (new_decl) = 0;
>>
>> Yes, that's what we had in mind. Though of course the changelog
>> doesn't match.
>>
>>
>> r~
>
> Ok, my English typing privileges have been revoked until further notice.
> I'm obviously typing cross-eyed right now.
>
> Here's what I have, but keep in mind that I'm going to watch Sesame
> Street so I'll be away from my keyboard for a few hours...
>
> PR middle-end/51411
> * trans-mem.c (ipa_tm_create_version): Do not zap TREE_PUBLIC.
>

This causes multiple definitions.

Extract of output with velox/glob2 benchmark:

src/AINicowar.o: In function `_ZGTtNSsD2Ev':
./include/c++/4.7.0/bits/basic_string.h:535: multiple definition of 
`_ZGTtNSsD2Ev'
src/AICastor.o:./include/c++/4.7.0/bits/basic_string.h:535: first 
defined here
src/AINicowar.o: In function `_ZGTtNKSs13get_allocatorEv':
./include/c++/4.7.0/bits/basic_string.h:1814: multiple definition of 
`_ZGTtNKSs13get_allocatorEv'
src/AICastor.o:./include/c++/4.7.0/bits/basic_string.h:1814: first 
defined here
src/AINicowar.o: In function `_ZGTtNKSs6_M_repEv':
./include/c++/4.7.0/bits/basic_string.h:297: multiple definition of 
`_ZGTtNKSs6_M_repEv'
src/AICastor.o:./include/c++/4.7.0/bits/basic_string.h:297: first 
defined here
src/AINicowar.o: In function `_ZGTtNSs4_Rep10_M_disposeERKSaIcE':
./include/c++/4.7.0/bits/basic_string.h:234: multiple definition of 
`_ZGTtNSs4_Rep10_M_disposeERKSaIcE'
src/AICastor.o:./include/c++/4.7.0/bits/basic_string.h:234: first 
defined here
src/AINicowar.o: In function `_ZGTtNSaIcED2Ev':
./include/c++/4.7.0/bits/allocator.h:112: multiple definition of 
`_ZGTtNSaIcED1Ev'
src/AICastor.o:./include/c++/4.7.0/bits/allocator.h:112: first defined here
src/AINicowar.o: In function `_ZGTtNSaIcEC2ERKS_':
./include/c++/4.7.0/bits/allocator.h:106: multiple definition of 
`_ZGTtNSaIcEC1ERKS_'
...

Without the patch, it is ok.
If I remove completely this part:
if (DECL_DECLARED_INLINE_P (new_decl) && DECL_EXTERNAL (new_decl))
   {
     DECL_EXTERNAL (new_decl) = 0;
     TREE_PUBLIC (new_decl) = 0;
   }
It ends with *only* one undefined symbol:
src/UnitsSkins.o: In function 
`_ZGTtStltIcSt11char_traitsIcESaIcEEbRKSbIT_T0_T1_ES8_':
./include/c++/4.7.0/bits/basic_string.h:2568: undefined reference to 
`_ZGTtNKSs7compareERKSs'

$ c++filt _ZNKSs7compareERKSs
std::basic_string<char, std::char_traits<char>, std::allocator<char> 
 >::compare(std::basic_string<char, std::char_traits<char>, 
std::allocator<char> > const&) const

I am not sure the way to give you useful information.

Patrick.
diff mbox

Patch

Index: testsuite/g++.dg/tm/pr51411.C
===================================================================
--- testsuite/g++.dg/tm/pr51411.C	(revision 0)
+++ testsuite/g++.dg/tm/pr51411.C	(revision 0)
@@ -0,0 +1,7 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm -O" }
+
+struct A
+{
+  __attribute__ ((transaction_safe)) virtual void virtfoo () { }
+};
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 182290)
+++ trans-mem.c	(working copy)
@@ -4256,10 +4256,7 @@  ipa_tm_create_version (struct cgraph_nod
       /* Remap extern inline to static inline.  */
       /* ??? Is it worth trying to use make_decl_one_only?  */
       if (DECL_DECLARED_INLINE_P (new_decl) && DECL_EXTERNAL (new_decl))
-	{
-	  DECL_EXTERNAL (new_decl) = 0;
-	  TREE_PUBLIC (new_decl) = 0;
-	}
+	DECL_EXTERNAL (new_decl) = 0;
 
       tree_function_versioning (old_decl, new_decl, NULL, false, NULL,
 				NULL, NULL);