Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On Mon, Nov 28, 2011 at 7:02 PM, Richard Henderson r...@redhat.com wrote: On 11/28/2011 03:54 PM, David Edelsohn wrote: On Wed, Nov 23, 2011 at 8:57 AM, Jakub Jelinek ja...@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. A patch using DECL_ONE_ONLY works for me as well. Can someone approve that version of the patch? * trans-mem.c (ipa_tm_create_version_alias): Mangle new_decl if DECL_ONE_ONLY. (ipa_tm_create_version): Same. Thanks, David Index: trans-mem.c === --- trans-mem.c (revision 181793) +++ trans-mem.c (working copy) @@ -4213,7 +4213,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 (DECL_ONE_ONLY (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); @@ -4248,7 +4248,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 (DECL_ONE_ONLY (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);
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On Tue, Nov 29, 2011 at 11:22:15AM -0500, David Edelsohn wrote: A patch using DECL_ONE_ONLY works for me as well. Can someone approve that version of the patch? This is ok for the trunk, thanks. * trans-mem.c (ipa_tm_create_version_alias): Mangle new_decl if DECL_ONE_ONLY. (ipa_tm_create_version): Same. --- trans-mem.c (revision 181793) +++ trans-mem.c (working copy) @@ -4213,7 +4213,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 (DECL_ONE_ONLY (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); @@ -4248,7 +4248,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 (DECL_ONE_ONLY (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); Jakub
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On Wed, Nov 23, 2011 at 8:57 AM, Jakub Jelinek ja...@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
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On 11/28/2011 03:54 PM, David Edelsohn wrote: On Wed, Nov 23, 2011 at 8:57 AM, Jakub Jelinek ja...@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.
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On Tue, Nov 22, 2011 at 8:06 AM, Aldy Hernandez al...@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
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
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 Hernandezal...@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
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
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
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
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? 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);
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
On Tue, Nov 22, 2011 at 8:06 AM, Aldy Hernandez al...@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
Re: PR other/51174: handle architectures with no DECL_COMDAT_GROUP
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~
