On 12/10/2013 04:48 AM, Jan Hubicka wrote:
The case where I played with local comdats was the following.
I made cgraph_body_availability to get context argument (i.e. instead of saying
if something is available in current unit, it was saying if it is available
in current function/variable).
If two symbols are in the same comdat group and refering each other, they are
available even though they may seem overwritable to others. I then started to
produce local symbols for those local references that are equivalent to your
comdat
local.
We probably want to get in this extension too. (I did not get data on how often
it fires, but it does i.e. for recursive calls of C++ inlines).
I wouldn't expect it to affect anything other than recursive calls,
since before my patch functions in the same COMDAT don't call each
other, and with my patch they only call functions that are already local.
Also, this optimization would seem to apply to all recursive functions,
not just those in comdat groups.
Are you thinking to add this after my patch is in?
+ /* Don't clone decls local to a comdat group. */
+ if (comdat_local_p (node))
+ return false;
Why? It seems this should work if the clone becomes another comdat local?
Right, I think the problem was that the clone wasn't becoming comdat
local. And for the specific case of decloning, there's no point in
cloning the decloned constructor.
On the other hand, I think you want to prevent ipa-cp propagating addresses of
comdat
locals out of the function. (i.e. make it to check can_refer predicate for its
subtitutions)
Right, I wasn't worrying about that because it can't come up with decloning.
So we should check here if both caller and callee are in the same group and
allow
inlining then?
Makes sense.
Probably you want to get make_decl_local to preserve comdat group; then you
won't need
to care about it here and clonning could work.
OK.
Probably also when declaring a comdat symbol local, we want to turn all
associated
comdats to local, right? (i.e. with LTO and hidden comdat)
I don't think so; if we change a public comdat symbol to local, that's a
change to the ABI of the comdat, so it can't be the same comdat anymore,
and dissolving the comdat seems appropriate.
Also i think this change needs more work. FROM_DECL is not the function you
are going to get the reference, it is variable whose constructor the value was
take from.
I think we need to rename it to can_refer_decl_in_symbol and add symbol argument
to get proper check. I am not sure where we use it without being sure the symbol
is current_function_decl. We definitly make use of it during devirt and we
need to
start using it in ipa-cp.
Would it be OK for me to just drop this change, since it isn't needed
for decloning?
Also this is not correct - because it ignores functions inlined into NODE and
use in can_inline_edge_p will lead to quadratic time issues. We probably want
ipa-inline to compute flag if function refer to local symbol just before
inlining starts and propagate it across inlining decisions in make_edge_inline
and only check the flag in can_inline_edge_p.
OK.
Jason
