| Message ID | 4ECB9E61.5020202@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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.
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));