* [PATCH] ipa-icf: skip variables with body_removed
@ 2022-05-19 7:08 Martin Liška
2022-05-19 15:02 ` Jan Hubicka
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2022-05-19 7:08 UTC (permalink / raw)
To: gcc-patches; +Cc: Jan Hubicka, Martin Jambor
Similarly to cgraph_nodes, it may happen that body_removed is set
during merging of symbols.
PR ipa/105600
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Thanks,
Martin
gcc/ChangeLog:
* ipa-icf.cc (sem_item_optimizer::filter_removed_items):
Skip variables with body_removed.
---
gcc/ipa-icf.cc | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
index 765ae746745..6528a7a10b2 100644
--- a/gcc/ipa-icf.cc
+++ b/gcc/ipa-icf.cc
@@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void)
{
/* Filter out non-readonly variables. */
tree decl = item->decl;
- if (TREE_READONLY (decl))
- filtered.safe_push (item);
- else
+ varpool_node *vnode = static_cast <sem_variable *>(item)->get_node ();
+ if (!TREE_READONLY (decl) || vnode->body_removed)
remove_item (item);
+ else
+ filtered.safe_push (item);
}
}
}
--
2.36.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipa-icf: skip variables with body_removed
2022-05-19 7:08 [PATCH] ipa-icf: skip variables with body_removed Martin Liška
@ 2022-05-19 15:02 ` Jan Hubicka
2022-05-20 7:46 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Jan Hubicka @ 2022-05-19 15:02 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Martin Jambor
> Similarly to cgraph_nodes, it may happen that body_removed is set
> during merging of symbols.
>
> PR ipa/105600
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> * ipa-icf.cc (sem_item_optimizer::filter_removed_items):
> Skip variables with body_removed.
> ---
> gcc/ipa-icf.cc | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
> index 765ae746745..6528a7a10b2 100644
> --- a/gcc/ipa-icf.cc
> +++ b/gcc/ipa-icf.cc
> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void)
> {
> /* Filter out non-readonly variables. */
> tree decl = item->decl;
> - if (TREE_READONLY (decl))
> - filtered.safe_push (item);
> - else
> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node ();
> + if (!TREE_READONLY (decl) || vnode->body_removed)
> remove_item (item);
> + else
> + filtered.safe_push (item);
So the situation here is that we merge symbols but keep syntactic alias
because the declarations are not compatible (have different attributes
perhaps because of fortify source)?
Will ICF still see through the aliases and do merging? I think you can
craft a testcase by triggering the attribute mismatch and see what
happens. At the time we implemented ICF these aliases did not exists,
so maybe some TLC is needed here.
Honza
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipa-icf: skip variables with body_removed
2022-05-19 15:02 ` Jan Hubicka
@ 2022-05-20 7:46 ` Martin Liška
2022-06-16 6:21 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2022-05-20 7:46 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, Martin Jambor
On 5/19/22 17:02, Jan Hubicka wrote:
>> Similarly to cgraph_nodes, it may happen that body_removed is set
>> during merging of symbols.
>>
>> PR ipa/105600
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> * ipa-icf.cc (sem_item_optimizer::filter_removed_items):
>> Skip variables with body_removed.
>> ---
>> gcc/ipa-icf.cc | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
>> index 765ae746745..6528a7a10b2 100644
>> --- a/gcc/ipa-icf.cc
>> +++ b/gcc/ipa-icf.cc
>> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void)
>> {
>> /* Filter out non-readonly variables. */
>> tree decl = item->decl;
>> - if (TREE_READONLY (decl))
>> - filtered.safe_push (item);
>> - else
>> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node ();
>> + if (!TREE_READONLY (decl) || vnode->body_removed)
>> remove_item (item);
>> + else
>> + filtered.safe_push (item);
>
> So the situation here is that we merge symbols but keep syntactic alias
> because the declarations are not compatible (have different attributes
> perhaps because of fortify source)?
The test-case is more about a constexpr symbol or so, I'm not familiar enough
with these modern C++.
cgraph_node looks like:
(gdb) p item->node->debug()
_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200
Type: variable
Body removed by symtab_remove_unreachable_nodes
Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only
References:
Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr)
Read from file: a.o
Availability: not_available
Varpool flags: initialized read-only
>
> Will ICF still see through the aliases and do merging?
No.
> I think you can
> craft a testcase by triggering the attribute mismatch and see what
> happens. At the time we implemented ICF these aliases did not exists,
> so maybe some TLC is needed here.
Please come up with such test case :)
Thanks,
Martin
>
> Honza
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipa-icf: skip variables with body_removed
2022-05-20 7:46 ` Martin Liška
@ 2022-06-16 6:21 ` Martin Liška
2022-06-22 10:56 ` Jan Hubicka
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2022-06-16 6:21 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, Martin Jambor
@Honza: PING
On 5/20/22 09:46, Martin Liška wrote:
> On 5/19/22 17:02, Jan Hubicka wrote:
>>> Similarly to cgraph_nodes, it may happen that body_removed is set
>>> during merging of symbols.
>>>
>>> PR ipa/105600
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>> Thanks,
>>> Martin
>>>
>>> gcc/ChangeLog:
>>>
>>> * ipa-icf.cc (sem_item_optimizer::filter_removed_items):
>>> Skip variables with body_removed.
>>> ---
>>> gcc/ipa-icf.cc | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
>>> index 765ae746745..6528a7a10b2 100644
>>> --- a/gcc/ipa-icf.cc
>>> +++ b/gcc/ipa-icf.cc
>>> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void)
>>> {
>>> /* Filter out non-readonly variables. */
>>> tree decl = item->decl;
>>> - if (TREE_READONLY (decl))
>>> - filtered.safe_push (item);
>>> - else
>>> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node ();
>>> + if (!TREE_READONLY (decl) || vnode->body_removed)
>>> remove_item (item);
>>> + else
>>> + filtered.safe_push (item);
>>
>> So the situation here is that we merge symbols but keep syntactic alias
>> because the declarations are not compatible (have different attributes
>> perhaps because of fortify source)?
>
> The test-case is more about a constexpr symbol or so, I'm not familiar enough
> with these modern C++.
>
> cgraph_node looks like:
>
> (gdb) p item->node->debug()
> _ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200
> Type: variable
> Body removed by symtab_remove_unreachable_nodes
> Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only
> References:
> Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr)
> Read from file: a.o
> Availability: not_available
> Varpool flags: initialized read-only
>
>
>>
>> Will ICF still see through the aliases and do merging?
>
> No.
>
>> I think you can
>> craft a testcase by triggering the attribute mismatch and see what
>> happens. At the time we implemented ICF these aliases did not exists,
>> so maybe some TLC is needed here.
>
> Please come up with such test case :)
>
> Thanks,
> Martin
>
>>
>> Honza
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipa-icf: skip variables with body_removed
2022-06-16 6:21 ` Martin Liška
@ 2022-06-22 10:56 ` Jan Hubicka
0 siblings, 0 replies; 5+ messages in thread
From: Jan Hubicka @ 2022-06-22 10:56 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Martin Jambor
> @Honza: PING
>
> On 5/20/22 09:46, Martin Liška wrote:
> > On 5/19/22 17:02, Jan Hubicka wrote:
> >>> Similarly to cgraph_nodes, it may happen that body_removed is set
> >>> during merging of symbols.
> >>>
> >>> PR ipa/105600
> >>>
> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>
> >>> Ready to be installed?
> >>> Thanks,
> >>> Martin
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> * ipa-icf.cc (sem_item_optimizer::filter_removed_items):
> >>> Skip variables with body_removed.
> >>> ---
> >>> gcc/ipa-icf.cc | 7 ++++---
> >>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
> >>> index 765ae746745..6528a7a10b2 100644
> >>> --- a/gcc/ipa-icf.cc
> >>> +++ b/gcc/ipa-icf.cc
> >>> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void)
> >>> {
> >>> /* Filter out non-readonly variables. */
> >>> tree decl = item->decl;
> >>> - if (TREE_READONLY (decl))
> >>> - filtered.safe_push (item);
> >>> - else
> >>> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node ();
> >>> + if (!TREE_READONLY (decl) || vnode->body_removed)
> >>> remove_item (item);
> >>> + else
> >>> + filtered.safe_push (item);
> >>
> >> So the situation here is that we merge symbols but keep syntactic alias
> >> because the declarations are not compatible (have different attributes
> >> perhaps because of fortify source)?
> >
> > The test-case is more about a constexpr symbol or so, I'm not familiar enough
> > with these modern C++.
> >
> > cgraph_node looks like:
> >
> > (gdb) p item->node->debug()
> > _ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200
> > Type: variable
> > Body removed by symtab_remove_unreachable_nodes
> > Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only
> > References:
> > Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr)
> > Read from file: a.o
> > Availability: not_available
> > Varpool flags: initialized read-only
OK, I think I worked out what happens here. The thing is that the
symbol has "external" flag set. In my mental picture we should be
keeping their definitions around since they are used for constant
folding, but it seems it is not what happens. We special case inline
functions but we do not special case external symbols.
This is a missed optimization oppurtunity but not completely trivial to
fix, since external symbol may reffer to internal one (such as comdat)
but even if it does so we want to optimize out that comdat. So we need
some bookkeeping for referneces from external symbols. I will implement
that but I guess we still want your patch since this is optional
optimization.
So patch is OK.
Honza
> >
> >
> >>
> >> Will ICF still see through the aliases and do merging?
> >
> > No.
> >
> >> I think you can
> >> craft a testcase by triggering the attribute mismatch and see what
> >> happens. At the time we implemented ICF these aliases did not exists,
> >> so maybe some TLC is needed here.
> >
> > Please come up with such test case :)
> >
> > Thanks,
> > Martin
> >
> >>
> >> Honza
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-22 10:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-19 7:08 [PATCH] ipa-icf: skip variables with body_removed Martin Liška
2022-05-19 15:02 ` Jan Hubicka
2022-05-20 7:46 ` Martin Liška
2022-06-16 6:21 ` Martin Liška
2022-06-22 10:56 ` Jan Hubicka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).