Patchwork PR other/51174: handle architectures with no DECL_COMDAT_GROUP

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 22, 2011, 1:06 p.m.
Message ID <4ECB9E61.5020202@redhat.com>
Download mbox | patch
Permalink /patch/127075/
State New
Headers show

Comments

Aldy Hernandez - Nov. 22, 2011, 1:06 p.m.
On 11/21/11 18:55, Richard Henderson wrote:
> On 11/18/2011 01:24 PM, Aldy Hernandez wrote:
>> -  if (DECL_COMDAT (new_decl))
>> +  if (DECL_COMDAT (new_decl)&&  HAVE_COMDAT_GROUP)
>>       DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>> +  else
>> +    DECL_COMDAT_GROUP (new_decl) = DECL_COMDAT_GROUP (old_decl);
>
> This looks weird -- you're seting D_C_G after H_C_G is false?
>
> We've already done copy_decl anyway -- you should be able to drop the else.
>
>
> r~

David, can you try the following and see if it fixes the problem on your 
end?

    new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
@@ -4233,7 +4233,7 @@ ipa_tm_create_version (struct cgraph_nod
    TREE_SYMBOL_REFERENCED (tm_name) = 1;

    /* Perform the same remapping to the comdat group.  */
-  if (DECL_COMDAT (new_decl))
+  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
      DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP 
(old_decl));

    new_node = cgraph_copy_node_for_versioning (old_node, new_decl, 
NULL, NULL);
David Edelsohn - Nov. 22, 2011, 1:58 p.m.
On Tue, Nov 22, 2011 at 8:06 AM, Aldy Hernandez <aldyh@redhat.com> wrote:

>> This looks weird -- you're seting D_C_G after H_C_G is false?
>>
>> We've already done copy_decl anyway -- you should be able to drop the
>> else.

> David, can you try the following and see if it fixes the problem on your
> end?
>
> Index: trans-mem.c
> ===================================================================
> --- trans-mem.c (revision 181588)
> +++ trans-mem.c (working copy)
> @@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>
>   /* Perform the same remapping to the comdat group.  */
> -  if (DECL_COMDAT (new_decl))
> +  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>
>   new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
> @@ -4233,7 +4233,7 @@ ipa_tm_create_version (struct cgraph_nod
>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>
>   /* Perform the same remapping to the comdat group.  */
> -  if (DECL_COMDAT (new_decl))
> +  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>
>   new_node = cgraph_copy_node_for_versioning (old_node, new_decl, NULL,
> NULL);
>

I assume this will work because AIX never uses the info in DECL_COMDAT_GROUP.

Note that ipa_tm_create_version() calls copy_node(), but
ipa_tm_create_version_alias() *DOES NOT*, it calls build_decl(), so
D_C_G will be garbage.

Yes, setting D_C_G looks strange with !H_C_G, but note that many
places in GCC blindly copy D_C_G or initialize it to 0.

- David
David Edelsohn - Nov. 23, 2011, 1:40 p.m.
On Tue, Nov 22, 2011 at 8:06 AM, Aldy Hernandez <aldyh@redhat.com> wrote:

> David, can you try the following and see if it fixes the problem on your
> end?
>
> Index: trans-mem.c
> ===================================================================
> --- trans-mem.c (revision 181588)
> +++ trans-mem.c (working copy)
> @@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>
>   /* Perform the same remapping to the comdat group.  */
> -  if (DECL_COMDAT (new_decl))
> +  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>
>   new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
> @@ -4233,7 +4233,7 @@ ipa_tm_create_version (struct cgraph_nod
>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>
>   /* Perform the same remapping to the comdat group.  */
> -  if (DECL_COMDAT (new_decl))
> +  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>
>   new_node = cgraph_copy_node_for_versioning (old_node, new_decl, NULL,
> NULL);
>

I successfully bootstrapped with this patch.

Thanks, David
Aldy Hernandez - Nov. 23, 2011, 1:47 p.m.
Richard, do you have an opinion on either one of these approaches?

Both bootstrap and regtest on x86-64 Linux and David AIX :-).  OK? 
Which one?

On 11/22/11 07:58, David Edelsohn wrote:
> On Tue, Nov 22, 2011 at 8:06 AM, Aldy Hernandez<aldyh@redhat.com>  wrote:
>
>>> This looks weird -- you're seting D_C_G after H_C_G is false?
>>>
>>> We've already done copy_decl anyway -- you should be able to drop the
>>> else.
>
>> David, can you try the following and see if it fixes the problem on your
>> end?
>>
>> Index: trans-mem.c
>> ===================================================================
>> --- trans-mem.c (revision 181588)
>> +++ trans-mem.c (working copy)
>> @@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
>>    TREE_SYMBOL_REFERENCED (tm_name) = 1;
>>
>>    /* Perform the same remapping to the comdat group.  */
>> -  if (DECL_COMDAT (new_decl))
>> +  if (HAVE_COMDAT_GROUP&&  DECL_COMDAT (new_decl))
>>      DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>>
>>    new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
>> @@ -4233,7 +4233,7 @@ ipa_tm_create_version (struct cgraph_nod
>>    TREE_SYMBOL_REFERENCED (tm_name) = 1;
>>
>>    /* Perform the same remapping to the comdat group.  */
>> -  if (DECL_COMDAT (new_decl))
>> +  if (HAVE_COMDAT_GROUP&&  DECL_COMDAT (new_decl))
>>      DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>>
>>    new_node = cgraph_copy_node_for_versioning (old_node, new_decl, NULL,
>> NULL);
>>
>
> I assume this will work because AIX never uses the info in DECL_COMDAT_GROUP.
>
> Note that ipa_tm_create_version() calls copy_node(), but
> ipa_tm_create_version_alias() *DOES NOT*, it calls build_decl(), so
> D_C_G will be garbage.
>
> Yes, setting D_C_G looks strange with !H_C_G, but note that many
> places in GCC blindly copy D_C_G or initialize it to 0.
>
> - David
Jakub Jelinek - Nov. 23, 2011, 1:57 p.m.
On Wed, Nov 23, 2011 at 07:47:46AM -0600, Aldy Hernandez wrote:
> >>@@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
> >>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
> >>
> >>   /* Perform the same remapping to the comdat group.  */
> >>-  if (DECL_COMDAT (new_decl))
> >>+  if (HAVE_COMDAT_GROUP&&  DECL_COMDAT (new_decl))
> >>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
> >>
> >>   new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);

Wouldn't it be better to test if (DECL_ONE_ONLY (new_decl))
instead?  That is actually test for non-NULL DECL_COMDAT_GROUP
and is what e.g. cp/ uses as guards on tweaking DECL_COMDAT_GROUP.

BTW, the formatting is wrong above, no space before && and two spaces after
it.

	Jakub
David Edelsohn - Nov. 28, 2011, 11:54 p.m.
On Wed, Nov 23, 2011 at 8:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 23, 2011 at 07:47:46AM -0600, Aldy Hernandez wrote:
>> >>@@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
>> >>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>> >>
>> >>   /* Perform the same remapping to the comdat group.  */
>> >>-  if (DECL_COMDAT (new_decl))
>> >>+  if (HAVE_COMDAT_GROUP&&  DECL_COMDAT (new_decl))
>> >>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>> >>
>> >>   new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
>
> Wouldn't it be better to test if (DECL_ONE_ONLY (new_decl))
> instead?  That is actually test for non-NULL DECL_COMDAT_GROUP
> and is what e.g. cp/ uses as guards on tweaking DECL_COMDAT_GROUP.
>
> BTW, the formatting is wrong above, no space before && and two spaces after
> it.

That will work as well.

Which solution should be committed?

Thanks, David
Richard Henderson - Nov. 29, 2011, 12:02 a.m.
On 11/28/2011 03:54 PM, David Edelsohn wrote:
> On Wed, Nov 23, 2011 at 8:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Nov 23, 2011 at 07:47:46AM -0600, Aldy Hernandez wrote:
>>>>> @@ -4198,7 +4198,7 @@ ipa_tm_create_version_alias (struct cgra
>>>>>   TREE_SYMBOL_REFERENCED (tm_name) = 1;
>>>>>
>>>>>   /* Perform the same remapping to the comdat group.  */
>>>>> -  if (DECL_COMDAT (new_decl))
>>>>> +  if (HAVE_COMDAT_GROUP&&  DECL_COMDAT (new_decl))
>>>>>     DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP (old_decl));
>>>>>
>>>>>   new_node = cgraph_same_body_alias (NULL, new_decl, info->new_decl);
>>
>> Wouldn't it be better to test if (DECL_ONE_ONLY (new_decl))
>> instead?  That is actually test for non-NULL DECL_COMDAT_GROUP
>> and is what e.g. cp/ uses as guards on tweaking DECL_COMDAT_GROUP.
>>
>> BTW, the formatting is wrong above, no space before && and two spaces after
>> it.
> 
> That will work as well.
> 
> Which solution should be committed?

Please use the DECL_ONE_ONLY test.

Patch

Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 181588)
+++ trans-mem.c	(working copy)
@@ -4198,7 +4198,7 @@  ipa_tm_create_version_alias (struct cgra
    TREE_SYMBOL_REFERENCED (tm_name) = 1;

    /* Perform the same remapping to the comdat group.  */
-  if (DECL_COMDAT (new_decl))
+  if (HAVE_COMDAT_GROUP && DECL_COMDAT (new_decl))
      DECL_COMDAT_GROUP (new_decl) = tm_mangle (DECL_COMDAT_GROUP 
(old_decl));