Hi, On Wed, Dec 11, 2013 at 02:55:26PM +0100, Jan Hubicka wrote: > > 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?
Uh, this will be quite a bit ugly but I will put it on my todo list. Nevertheless, if you believe that a particular function should not be cloned by ipa-cp, the best place to do make that change is in ipcp_versionable_function_p, decide_about_value is too late and a lot of assumptions about versionablity have already been made when it runs. Thanks, Martin
