> 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.
Agreed, I already do the conversion for many functions (based on "inline" keyword implying that there is no overwrite changing semantic). So far the conversion does not happen for comdats, since it would lead to local comdat and I also ignored the conversion rule. I have patch for that that handles it post-inlining + inliner patch that takes advantage of function context to allow recursive inlining. > > Are you thinking to add this after my patch is in? Yes, lets do that incrementally. > > >>+ /* 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. If it does not make sense, how we ended up cloning it? Can you show me some code sample of decloning? I assume that we basically turn original cloned constructors into wrappers around common "worker" that is now comdat local. I think ipa-cp may end up deciding on clonning the wrapper that will break because it will end up static calling the local comdat function. On the other hand it may decide to clone both the wrapper and the actual function and in that case bringing both static is fine. So to be on safe side, we probably want to consider functions calling local comdat unclonable but we do not need to worry about local comdats themselves. For good codegen, I think ipa-cp will need to understand it needs to either clone both or nothing. I think it is something Martin can look into? > > >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. Yep, this is what I had in mind. When we declare to turn comdat into non-comdat we need to make sure that the comdat locals are dissolved, too. > > >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? OK, I will make followup patch for this. Thanks, Honza
