Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 27, 2018, at 3:58 AM, Jan Hubicka  wrote:
> 
>> 
>> Okay, I see.
>> 
>>> 
>>> If you make this to be INTERPOSABLE (which means it can be replaced by 
>>> different
>>> implementation by linker and that is probably what we want for live 
>>> patching)
>>> then also inliner, ipa-sra and other optimization will give up on these.
>> 
>> do you suggest that to set the global function as AVAIL_INTERPOSABLE when 
>> -finline-only-static 
>> is present? then we should avoid all issues?
> 
> It seems to be reasonable direction I think, because it is what really happens
> (well AVAIL_INTERPOSABLE still does not assume that the interposition will
> happen at runtime, but it is an approximation and we may introduce something 
> like
> AVAIL_RUNTIME_INTERPOSABLE if there is need for better difference).
> I wonder if -finline-only-static is good name for the flag though, because it
> does a lot more than that.  Maybe something like -flive-patching?
> How much is this all tied to one particular implementation of the feature?

Yes, I like this idea. I will study a little more on this direction and report 
back.

Qing
> 
> Honza
>> 
>> Qing
>> 

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 27, 2018, at 2:45 AM, Richard Biener  wrote:
> 
> On Wed, 26 Sep 2018, Qing Zhao wrote:
> 
>> 
>>> On Sep 26, 2018, at 6:07 PM, Alexander Monakov  wrote:
>>> 
>>> On Wed, 26 Sep 2018, Qing Zhao wrote:
 The request is for application developers who want to use gcc's online
  patching feature.
 
 Today, developers can turn off inlining and deliver just the patched 
 routine.  They
  can also allow all inlining and deliver the patched routine and all the 
 routines
  that the patched routine was inlined into.
 
 completely turning off inlining will sacrifice too much run-time 
 performance. completely
 enable inlining, on the other hand, will have the potential issues with 
 code size, complexity and
 debuggability for the online patching.
 
 the proposed option provides a compromised solution for the above issues. 
 and enable more
 developers to utilize gcc’s online patching feature.
>>> 
>>> From this explanation it sounds to me that what you really need is -Os-like
>>> behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
>>> mentioned in my previous email. Honza, how does that sound?
>> 
>> I don’t think a -Os-like option will do the job.
>> 
>> As Jeff already mentioned in a very previous email:
>> 
>> “Presumably one of the cases where this capability is really helpful is
>> things like ksplice.   If you have a function with global scope that has
>> been potentially inlined, then it's a lot harder track down those
>> inlining points at DTRT.
>> 
>> We ran into this internally when looking at hot patching some of the
>> spinlock code in the kernel.  It would have been real helpful if the
>> kernel had been compiled with this kind of option :-)
>> 
>> So conceptually I can see value in this kind of option.
>> “
>> 
>> so, specially control inlining on static/global will be helpful to online 
>> patch.
> 
> But as Honza said this gets you only sofar.  IIRC for our kernel 
> livepatching we turn off most IPA passes because while we can "easily"
> figure what and where things were inlined spotting the effects of
> IPA analysis and transform is almost impossible.
> 
> So there's two parts of the knob - one is to make the live-patch size
> not explode (do less inlining where it doesn't hurt performance - that
> eventually also applies to static functions called once inlining!).

Yes, limit additional unnecessary inlining might be better/

> The other is to make it possible to conservatively compute the set
> of functions you have to replace (the set of functions that are
> affected by a patch).
right.
> 
> Having an option to _that_ effect might indeed be useful (to avoid
> people chasing all the flags they need to disable).  So shouldn't this
> be a -fease-live-patching option rather that -finline-only-static
> which doesn't really capture the intention nor the effect?
Yes, if we can have -fease-live-patching, that will be even better for the live 
patch purpose.

> 
> That is, -fease-live-patching would guarantee that if you source-patch
> function X then, if you replace all functions which debuginfo tells you
> X was inlined to, the result will be semantically equivalent with
> replacing the whole program?  

Okay.

> We might even add sth like
> -fpatch-symbol-list=FOO,BAR that outputs a list of symbols into BAR
> that are affected this way when functions FOO are changed (you
> run that on unpatched code of course).  Or we add sth to the
> cgraph dumpfile that for each function lists the set of symbols
> it was affected by.

Yes, this option might be also helpful for livepatching. we can do it at the 
next step.

Qing
> 
> Thanks,
> Richard.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Richard Biener]

On Thu, 27 Sep 2018, Jan Hubicka wrote:

> > On Wed, 26 Sep 2018, Qing Zhao wrote:
> > > The request is for application developers who want to use gcc's online
> > >patching feature.
> > > 
> > > Today, developers can turn off inlining and deliver just the patched 
> > > routine.  They
> > >can also allow all inlining and deliver the patched routine and all 
> > > the routines
> > >that the patched routine was inlined into.
> > > 
> > > completely turning off inlining will sacrifice too much run-time 
> > > performance. completely
> > > enable inlining, on the other hand, will have the potential issues with 
> > > code size, complexity and
> > > debuggability for the online patching.
> > > 
> > > the proposed option provides a compromised solution for the above issues. 
> > > and enable more
> > > developers to utilize gcc’s online patching feature.
> > 
> > From this explanation it sounds to me that what you really need is -Os-like
> > behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
> > mentioned in my previous email. Honza, how does that sound?
> 
> How -Os is related? We will still do things like inlining or cloning of 
> functions
> if we expect code size to decrease (that may happen if arguments become dead)

Yeah, I was suggesting -fno-inline-small-functions which would get you
to do it the Linus-way (do-what-I-say) and only inline 'inline' declared
functions.

Richard.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jan Hubicka]

> 
> Okay, I see.
> 
> > 
> > If you make this to be INTERPOSABLE (which means it can be replaced by 
> > different
> > implementation by linker and that is probably what we want for live 
> > patching)
> > then also inliner, ipa-sra and other optimization will give up on these.
> 
> do you suggest that to set the global function as AVAIL_INTERPOSABLE when 
> -finline-only-static 
> is present? then we should avoid all issues?

It seems to be reasonable direction I think, because it is what really happens
(well AVAIL_INTERPOSABLE still does not assume that the interposition will
happen at runtime, but it is an approximation and we may introduce something 
like
AVAIL_RUNTIME_INTERPOSABLE if there is need for better difference).
I wonder if -finline-only-static is good name for the flag though, because it
does a lot more than that.  Maybe something like -flive-patching?
How much is this all tied to one particular implementation of the feature?

Honza
> 
> Qing
> 
> 
> > Honza
> >> 
> >> 
> >>> For example comdat that was cloned by IPA-SRA. See can_be_local_p and
> >>> comdat_can_be_unshared_p predicates.  Similar problem happens to clones 
> >>> created
> >>> by ipa-cp.
> >>> 
> >>> I guess we want to disable localization and cloning in this case as well.
> >>> I wonder what else.
> >> 
> >> Yes, I think we should make -finline-only-static incompatible with cloning 
> >> and tree-sra too.
> >> 
> >> Qing
> >>> 
> >>> Honza
> 

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jan Hubicka]

> On Wed, 26 Sep 2018, Qing Zhao wrote:
> > The request is for application developers who want to use gcc's online
> >patching feature.
> > 
> > Today, developers can turn off inlining and deliver just the patched 
> > routine.  They
> >can also allow all inlining and deliver the patched routine and all the 
> > routines
> >that the patched routine was inlined into.
> > 
> > completely turning off inlining will sacrifice too much run-time 
> > performance. completely
> > enable inlining, on the other hand, will have the potential issues with 
> > code size, complexity and
> > debuggability for the online patching.
> > 
> > the proposed option provides a compromised solution for the above issues. 
> > and enable more
> > developers to utilize gcc’s online patching feature.
> 
> From this explanation it sounds to me that what you really need is -Os-like
> behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
> mentioned in my previous email. Honza, how does that sound?

How -Os is related? We will still do things like inlining or cloning of 
functions
if we expect code size to decrease (that may happen if arguments become dead)

Honza
> 
> > > If the original issue is that inlining duplicates code, wouldn't it be 
> > > better
> > > solved by a switch that instructs inlining heuristics to inline as if for 
> > > -Os,
> > > without enabling -Os for other passes?
> 
> Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Alexander Monakov]

On Wed, 26 Sep 2018, Qing Zhao wrote:
> I don’t think a -Os-like option will do the job.
> 
> As Jeff already mentioned in a very previous email:
> 
> “Presumably one of the cases where this capability is really helpful is
> things like ksplice.   If you have a function with global scope that has
> been potentially inlined, then it's a lot harder track down those
> inlining points at DTRT.
> 
> We ran into this internally when looking at hot patching some of the
> spinlock code in the kernel.  It would have been real helpful if the
> kernel had been compiled with this kind of option :-)
> 
> So conceptually I can see value in this kind of option.
> “
> 
> so, specially control inlining on static/global will be helpful to online 
> patch.

In addition to what Richard said, I don't follow this reasoning. The issue with
spinlock functions is that many of them are defined in header files and get
inlined everywhere, correct? But such functions are all 'static inline', so this
patch wouldn't affect them at all.

Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Richard Biener]

On Wed, 26 Sep 2018, Qing Zhao wrote:

> 
> > On Sep 26, 2018, at 6:07 PM, Alexander Monakov  wrote:
> > 
> > On Wed, 26 Sep 2018, Qing Zhao wrote:
> >> The request is for application developers who want to use gcc's online
> >>   patching feature.
> >> 
> >> Today, developers can turn off inlining and deliver just the patched 
> >> routine.  They
> >>   can also allow all inlining and deliver the patched routine and all the 
> >> routines
> >>   that the patched routine was inlined into.
> >> 
> >> completely turning off inlining will sacrifice too much run-time 
> >> performance. completely
> >> enable inlining, on the other hand, will have the potential issues with 
> >> code size, complexity and
> >> debuggability for the online patching.
> >> 
> >> the proposed option provides a compromised solution for the above issues. 
> >> and enable more
> >> developers to utilize gcc’s online patching feature.
> > 
> > From this explanation it sounds to me that what you really need is -Os-like
> > behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
> > mentioned in my previous email. Honza, how does that sound?
> 
> I don’t think a -Os-like option will do the job.
> 
> As Jeff already mentioned in a very previous email:
> 
> “Presumably one of the cases where this capability is really helpful is
> things like ksplice.   If you have a function with global scope that has
> been potentially inlined, then it's a lot harder track down those
> inlining points at DTRT.
> 
> We ran into this internally when looking at hot patching some of the
> spinlock code in the kernel.  It would have been real helpful if the
> kernel had been compiled with this kind of option :-)
> 
> So conceptually I can see value in this kind of option.
> “
> 
> so, specially control inlining on static/global will be helpful to online 
> patch.

But as Honza said this gets you only sofar.  IIRC for our kernel 
livepatching we turn off most IPA passes because while we can "easily"
figure what and where things were inlined spotting the effects of
IPA analysis and transform is almost impossible.

So there's two parts of the knob - one is to make the live-patch size
not explode (do less inlining where it doesn't hurt performance - that
eventually also applies to static functions called once inlining!).
The other is to make it possible to conservatively compute the set
of functions you have to replace (the set of functions that are
affected by a patch).

Having an option to _that_ effect might indeed be useful (to avoid
people chasing all the flags they need to disable).  So shouldn't this
be a -fease-live-patching option rather that -finline-only-static
which doesn't really capture the intention nor the effect?

That is, -fease-live-patching would guarantee that if you source-patch
function X then, if you replace all functions which debuginfo tells you
X was inlined to, the result will be semantically equivalent with
replacing the whole program?  We might even add sth like
-fpatch-symbol-list=FOO,BAR that outputs a list of symbols into BAR
that are affected this way when functions FOO are changed (you
run that on unpatched code of course).  Or we add sth to the
cgraph dumpfile that for each function lists the set of symbols
it was affected by.

Thanks,
Richard.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 6:07 PM, Alexander Monakov  wrote:
> 
> On Wed, 26 Sep 2018, Qing Zhao wrote:
>> The request is for application developers who want to use gcc's online
>>   patching feature.
>> 
>> Today, developers can turn off inlining and deliver just the patched 
>> routine.  They
>>   can also allow all inlining and deliver the patched routine and all the 
>> routines
>>   that the patched routine was inlined into.
>> 
>> completely turning off inlining will sacrifice too much run-time 
>> performance. completely
>> enable inlining, on the other hand, will have the potential issues with code 
>> size, complexity and
>> debuggability for the online patching.
>> 
>> the proposed option provides a compromised solution for the above issues. 
>> and enable more
>> developers to utilize gcc’s online patching feature.
> 
> From this explanation it sounds to me that what you really need is -Os-like
> behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
> mentioned in my previous email. Honza, how does that sound?

I don’t think a -Os-like option will do the job.

As Jeff already mentioned in a very previous email:

“Presumably one of the cases where this capability is really helpful is
things like ksplice.   If you have a function with global scope that has
been potentially inlined, then it's a lot harder track down those
inlining points at DTRT.

We ran into this internally when looking at hot patching some of the
spinlock code in the kernel.  It would have been real helpful if the
kernel had been compiled with this kind of option :-)

So conceptually I can see value in this kind of option.
“

so, specially control inlining on static/global will be helpful to online patch.

Qing

> 
>>> If the original issue is that inlining duplicates code, wouldn't it be 
>>> better
>>> solved by a switch that instructs inlining heuristics to inline as if for 
>>> -Os,
>>> without enabling -Os for other passes?
> 
> Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Alexander Monakov]

On Wed, 26 Sep 2018, Qing Zhao wrote:
> The request is for application developers who want to use gcc's online
>patching feature.
> 
> Today, developers can turn off inlining and deliver just the patched routine. 
>  They
>can also allow all inlining and deliver the patched routine and all the 
> routines
>that the patched routine was inlined into.
> 
> completely turning off inlining will sacrifice too much run-time performance. 
> completely
> enable inlining, on the other hand, will have the potential issues with code 
> size, complexity and
> debuggability for the online patching.
> 
> the proposed option provides a compromised solution for the above issues. and 
> enable more
> developers to utilize gcc’s online patching feature.

>From this explanation it sounds to me that what you really need is -Os-like
behavior for IPA passes, without enabling -Os for gimple/rtl passes, as I
mentioned in my previous email. Honza, how does that sound?

> > If the original issue is that inlining duplicates code, wouldn't it be 
> > better
> > solved by a switch that instructs inlining heuristics to inline as if for 
> > -Os,
> > without enabling -Os for other passes?

Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 10:16 AM, Alexander Monakov  wrote:
> 
> On Wed, 26 Sep 2018, Qing Zhao wrote:
> 
>> Alexander,
>> 
>> thanks for the questions.
>> 
>> Yes, we had some discussion on the questions you raised during the review of 
>> the initial patch back to 9/11/2018.
>> please take a look at those discussions at:
>> 
>> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00549.html 
>> 
>> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00787.html 
>> 
>> 
>> and let me know if those discussion still does not answer your questions.
> 
> Thank you. Yes, it is still unclear to me why restricting inlining to static
> functions noticeably helps in your case. Is it because you build the kernel
> with LTO? Otherwise effects from inlining are limited to one compilation unit,
> except for functions defined in headers. But for those, the kernel
> uses 'static inline' anyway, so the patch wouldn't change anything.

The request is for application developers who want to use gcc's online
   patching feature.

Today, developers can turn off inlining and deliver just the patched routine.  
They
   can also allow all inlining and deliver the patched routine and all the 
routines
   that the patched routine was inlined into.

completely turning off inlining will sacrifice too much run-time performance. 
completely
enable inlining, on the other hand, will have the potential issues with code 
size, complexity and
debuggability for the online patching.

the proposed option provides a compromised solution for the above issues. and 
enable more
developers to utilize gcc’s online patching feature.

hope this is helpful.

Qing


> If the original issue is that inlining duplicates code, wouldn't it be better
> solved by a switch that instructs inlining heuristics to inline as if for -Os,
> without enabling -Os for other passes?
> 
> Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 12:16 PM, Jan Hubicka  wrote:
> 
>>> 
>>> On Sep 26, 2018, at 10:06 AM, Jan Hubicka  wrote:
>>> 
 On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law  wrote:
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  
>> wrote:
>>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
>>> what 'internal' would mean in this context.
>> 
>> I mean internal linkage as in the C and C++ standards.
 
> Since this is primarily for kernel hot patching, I think we're looking
> to restrict inlining to functions that have visibility limited to a
> compilation unit.
 
 Right, which is internal linkage.
 
 C11: "Within one translation unit, each declaration of an identifier
 with internal linkage denotes the same object or function."
 C++17: "When a name has internal linkage, the entity it denotes can be
 referred to by names from other scopes in the same translation unit."
 
 Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
 richi suggested.
>>> 
>>> I am not quite sure if we can relate visibility flags we have at this stage
>>> to visibility in source languge in very coherent way.  They change a lot 
>>> with
>>> LTO and we may want to make this option incompatible with LTO, but even w/o
>>> we can turn function static that previously wasn’t.
>> 
>> Looks like both LTO and whole_program need to be made incompatible with 
>> -finline-only-static. 
>> from my study of the function “cgraph_externally_visible_p”, comdat 
>> functions can ONLY be turned into
>> static when either “in_lto_p” or “whole_program” is true. 
> 
> This is not quite the case, because, we can still clone them to static 
> functions
> or derive fact about their side effects (such that information if they 
> write/read
> memory, particular global var etc).  All this inter-procedural propagation
> is guarded by get_availability predicate returning at least AVAILABLE.

Okay, I see.

> 
> If you make this to be INTERPOSABLE (which means it can be replaced by 
> different
> implementation by linker and that is probably what we want for live patching)
> then also inliner, ipa-sra and other optimization will give up on these.

do you suggest that to set the global function as AVAIL_INTERPOSABLE when 
-finline-only-static 
is present? then we should avoid all issues?

Qing


> Honza
>> 
>> 
>>> For example comdat that was cloned by IPA-SRA. See can_be_local_p and
>>> comdat_can_be_unshared_p predicates.  Similar problem happens to clones 
>>> created
>>> by ipa-cp.
>>> 
>>> I guess we want to disable localization and cloning in this case as well.
>>> I wonder what else.
>> 
>> Yes, I think we should make -finline-only-static incompatible with cloning 
>> and tree-sra too.
>> 
>> Qing
>>> 
>>> Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 12:16 PM, Jan Hubicka  > wrote:
> 
>>> 
>>> On Sep 26, 2018, at 10:06 AM, Jan Hubicka >> > wrote:
>>> 
 On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law >>> > wrote:
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener > > wrote:
>>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
>>> what 'internal' would mean in this context.
>> 
>> I mean internal linkage as in the C and C++ standards.
 
> Since this is primarily for kernel hot patching, I think we're looking
> to restrict inlining to functions that have visibility limited to a
> compilation unit.
 
 Right, which is internal linkage.
 
 C11: "Within one translation unit, each declaration of an identifier
 with internal linkage denotes the same object or function."
 C++17: "When a name has internal linkage, the entity it denotes can be
 referred to by names from other scopes in the same translation unit."
 
 Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
 richi suggested.
>>> 
>>> I am not quite sure if we can relate visibility flags we have at this stage
>>> to visibility in source languge in very coherent way.  They change a lot 
>>> with
>>> LTO and we may want to make this option incompatible with LTO, but even w/o
>>> we can turn function static that previously wasn’t.
>> 
>> Looks like both LTO and whole_program need to be made incompatible with 
>> -finline-only-static. 
>> from my study of the function “cgraph_externally_visible_p”, comdat 
>> functions can ONLY be turned into
>> static when either “in_lto_p” or “whole_program” is true. 
> 
> This is not quite the case, because, we can still clone them to static 
> functions
> or derive fact about their side effects (such that information if they 
> write/read
> memory, particular global var etc).  All this inter-procedural propagation
> is guarded by get_availability predicate returning at least AVAILABLE.

Okay, I see.

> 
> If you make this to be INTERPOSABLE (which means it can be replaced by 
> different
> implementation by linker and that is probably what we want for live patching)
> then also inliner, ipa-sra and other optimization will give up on these.

do you suggest that to set the global function as AVAIL_INTERPOSABLE when 
-finline-only-static 
is present? then we should avoid all issues?

Qing


> Honza
>> 
>> 
>>> For example comdat that was cloned by IPA-SRA. See can_be_local_p and
>>> comdat_can_be_unshared_p predicates.  Similar problem happens to clones 
>>> created
>>> by ipa-cp.
>>> 
>>> I guess we want to disable localization and cloning in this case as well.
>>> I wonder what else.
>> 
>> Yes, I think we should make -finline-only-static incompatible with cloning 
>> and tree-sra too.
>> 
>> Qing
>>> 
>>> Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 11:02 AM, Jan Hubicka  wrote:
> 
>>> Not sure
>>> what 'internal' would mean in this context.
>>> 
>>> But then the implementation looks at callee->externally_visible which
>>> matches hidden visibility... externally_visible is probably not
>>> the very best thing to look at depending on what we intend to do.
>> 
>> from the comments of callee->externally_visible in cgraph.h:
>> 
>>  /* Set when function is visible by other units.  */
>>  unsigned externally_visible : 1;
>> 
>> My understand of this “externally_visible” is:
>> 
>> this function is visible from other compilation units.
>> 
>> Is this correct?
> 
> Yes, but the catch is that we may "localize" previously visible function into
> invisible by clonning, see my previous email.

Yes, these are the cases that we need to take care carefully.

>>> Note you shouldn't look at individual cgraph_node fields but use
>>> some of the accessors with more well-constrained semantics.
>>> Why are you not simply checking !TREE_PUBLIC?
>> 
>> Yes, looks like that TREE_PUBLIC(node->decl) might be better for this 
>> purpose.
> 
> externally_visible is better approximation, TREE_PUBLIC is false i.e. for
> weakref aliases.  But we need to check places where externally visible 
> functions
> are turned to local.

So, do you suggest to make all the optimizations that might turn external 
visible functions into local 
as incompatible with -finline-only-static?


(a possible list of such optimizations include cloning, ipa-cp, ipa-sra, etc?)

> 
> What about comdats in general? What is the intended behaviour? If you prevent
> inlining them completely, C++ programs will be very slow.
> Also we still can analyze the body and derive some facts about them to drive
> optimization (such as discovering that they are const/pure etc). Do we want
> to disable this too?

my current understanding of comdat functions is:  they are external visible 
initially, but under
some special situation, (for example, comdat_can_be_unshared_p + in_lto_p), 
they can 
be converted to local by the compiler. 

so, for most of the comdat functions, they are NOT inlined by the current 
implementation if -finline-only-static
is specified due to they are externally_visible.  

I am not sure whether it’s necessary to change this current behavior for comdat 
functions, I assume that most
of the applications for online patching are not written by C++,  not sure 
whether this assumption is reasonable or not?

any suggestion?

thanks.

Qing



> Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jan Hubicka]

> 
> > On Sep 26, 2018, at 10:06 AM, Jan Hubicka  wrote:
> > 
> >> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law  wrote:
> >>> On 9/26/18 7:38 AM, Jason Merrill wrote:
>  On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  
>  wrote:
> > IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
> > what 'internal' would mean in this context.
>  
>  I mean internal linkage as in the C and C++ standards.
> >> 
> >>> Since this is primarily for kernel hot patching, I think we're looking
> >>> to restrict inlining to functions that have visibility limited to a
> >>> compilation unit.
> >> 
> >> Right, which is internal linkage.
> >> 
> >> C11: "Within one translation unit, each declaration of an identifier
> >> with internal linkage denotes the same object or function."
> >> C++17: "When a name has internal linkage, the entity it denotes can be
> >> referred to by names from other scopes in the same translation unit."
> >> 
> >> Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
> >> richi suggested.
> > 
> > I am not quite sure if we can relate visibility flags we have at this stage
> > to visibility in source languge in very coherent way.  They change a lot 
> > with
> > LTO and we may want to make this option incompatible with LTO, but even w/o
> > we can turn function static that previously wasn’t.
> 
> Looks like both LTO and whole_program need to be made incompatible with 
> -finline-only-static. 
> from my study of the function “cgraph_externally_visible_p”, comdat functions 
> can ONLY be turned into
> static when either “in_lto_p” or “whole_program” is true. 

This is not quite the case, because, we can still clone them to static functions
or derive fact about their side effects (such that information if they 
write/read
memory, particular global var etc).  All this inter-procedural propagation
is guarded by get_availability predicate returning at least AVAILABLE.

If you make this to be INTERPOSABLE (which means it can be replaced by different
implementation by linker and that is probably what we want for live patching)
then also inliner, ipa-sra and other optimization will give up on these.

Honza
> 
> 
> > For example comdat that was cloned by IPA-SRA. See can_be_local_p and
> > comdat_can_be_unshared_p predicates.  Similar problem happens to clones 
> > created
> > by ipa-cp.
> > 
> > I guess we want to disable localization and cloning in this case as well.
> > I wonder what else.
> 
> Yes, I think we should make -finline-only-static incompatible with cloning 
> and tree-sra too.
> 
> Qing
> > 
> > Honza
> 

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 10:06 AM, Jan Hubicka  wrote:
> 
>> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law  wrote:
>>> On 9/26/18 7:38 AM, Jason Merrill wrote:
 On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
> what 'internal' would mean in this context.
 
 I mean internal linkage as in the C and C++ standards.
>> 
>>> Since this is primarily for kernel hot patching, I think we're looking
>>> to restrict inlining to functions that have visibility limited to a
>>> compilation unit.
>> 
>> Right, which is internal linkage.
>> 
>> C11: "Within one translation unit, each declaration of an identifier
>> with internal linkage denotes the same object or function."
>> C++17: "When a name has internal linkage, the entity it denotes can be
>> referred to by names from other scopes in the same translation unit."
>> 
>> Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
>> richi suggested.
> 
> I am not quite sure if we can relate visibility flags we have at this stage
> to visibility in source languge in very coherent way.  They change a lot with
> LTO and we may want to make this option incompatible with LTO, but even w/o
> we can turn function static that previously wasn’t.

Looks like both LTO and whole_program need to be made incompatible with 
-finline-only-static. 
from my study of the function “cgraph_externally_visible_p”, comdat functions 
can ONLY be turned into
static when either “in_lto_p” or “whole_program” is true. 


> For example comdat that was cloned by IPA-SRA. See can_be_local_p and
> comdat_can_be_unshared_p predicates.  Similar problem happens to clones 
> created
> by ipa-cp.
> 
> I guess we want to disable localization and cloning in this case as well.
> I wonder what else.

Yes, I think we should make -finline-only-static incompatible with cloning and 
tree-sra too.

Qing
> 
> Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jan Hubicka]

> >  Not sure
> > what 'internal' would mean in this context.
> > 
> > But then the implementation looks at callee->externally_visible which
> > matches hidden visibility... externally_visible is probably not
> > the very best thing to look at depending on what we intend to do.
> 
> from the comments of callee->externally_visible in cgraph.h:
> 
>   /* Set when function is visible by other units.  */
>   unsigned externally_visible : 1;
> 
> My understand of this “externally_visible” is:
> 
> this function is visible from other compilation units.
> 
> Is this correct?

Yes, but the catch is that we may "localize" previously visible function into
invisible by clonning, see my previous email.
> > Note you shouldn't look at individual cgraph_node fields but use
> > some of the accessors with more well-constrained semantics.
> > Why are you not simply checking !TREE_PUBLIC?
> 
> Yes, looks like that TREE_PUBLIC(node->decl) might be better for this purpose.

externally_visible is better approximation, TREE_PUBLIC is false i.e. for
weakref aliases.  But we need to check places where externally visible functions
are turned to local.

What about comdats in general? What is the intended behaviour? If you prevent
inlining them completely, C++ programs will be very slow.
Also we still can analyze the body and derive some facts about them to drive
optimization (such as discovering that they are const/pure etc). Do we want
to disable this too?

Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 9:45 AM, Jeff Law  wrote:
> 
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
>>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
>>> what 'internal' would mean in this context.
>> 
>> I mean internal linkage as in the C and C++ standards.
> Since this is primarily for kernel hot patching, I think we're looking
> to restrict inlining to functions that have visibility limited to a
> compilation unit.

Yes. that’s the intention.

-finline-only-static will ONLY inline functions that are visible within the 
current compilation unit.

Qing
> 
> Qing, can you confirm that either way?
> 
> Jeff

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

> On Sep 26, 2018, at 8:24 AM, Richard Biener  wrote:
> 
> On Wed, 26 Sep 2018, Jason Merrill wrote:
> 
>> On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao  wrote:
>>> Hi, this is the 4th version of the patch.
>>> 
>>> mainly address Martin’s comments on some spelling issues.
>>> 
>>> I have tested the patch on both x86 and aarch64, no issue.
>>> 
>>> Okay for commit?
>>> 
>>> thanks.
>>> 
>>> Qing.
>>> 
>>> gcc/ChangeLog
>>> 
>>> +2018-09-20  Qing Zhao  
>>> +
>>> +   * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
>>> +   * common.opt (-finline-only-static): New option.
>>> +   * doc/invoke.texi: Document -finline-only-static.
>>> +   * ipa-inline.c (can_inline_edge_p): Control inlining based on
>>> +   function's linkage.
>> 
>> I would suggest "internal" rather than "static" in general.  So
>> 
>> +N_("function has external linkage when the user requests only"
>> +   " inlining functions with internal linkage"))
>> 
>> +Inline functions only if they have internal linkage.
>> 
>> +@item -finline-only-internal
>> +@opindex finline-only-internal
>> +By default, GCC inlines functions without considering their linkage.
>> +This flag guides the inliner to only inline functions with internal linkage.
>> +This option has any effect only when inlining itself is turned on by the
>> +-finline-functions or -finline-small-functions options.
>> 
>> This should also mention whether it applies to functions explicitly
>> declared inline; I assume it does not.
> 
> IIRC he explicitely wanted 'static' not 'hidden' linkage.

Yes.  that’s the intention. It will be very helpful to compile the application 
with ONLY inlining
STATIC functions for online-patching purpose. 

>  Not sure
> what 'internal' would mean in this context.
> 
> But then the implementation looks at callee->externally_visible which
> matches hidden visibility... externally_visible is probably not
> the very best thing to look at depending on what we intend to do.

from the comments of callee->externally_visible in cgraph.h:

  /* Set when function is visible by other units.  */
  unsigned externally_visible : 1;

My understand of this “externally_visible” is:

this function is visible from other compilation units.

Is this correct?

> 
> What about 'static' functions with their address taken?
> 

such functions should still be inlined if -finline-only-static is specified. 

is there any issue with this?

> Note you shouldn't look at individual cgraph_node fields but use
> some of the accessors with more well-constrained semantics.
> Why are you not simply checking !TREE_PUBLIC?

Yes, looks like that TREE_PUBLIC(node->decl) might be better for this purpose.

> 
> Honza might be able to suggest one.

thanks.

Qing
> 
> Richard.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Alexander Monakov]

On Wed, 26 Sep 2018, Qing Zhao wrote:

> Alexander,
> 
> thanks for the questions.
> 
> Yes, we had some discussion on the questions you raised during the review of 
> the initial patch back to 9/11/2018.
> please take a look at those discussions at:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00549.html 
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00787.html 
> 
> 
> and let me know if those discussion still does not answer your questions.

Thank you. Yes, it is still unclear to me why restricting inlining to static
functions noticeably helps in your case. Is it because you build the kernel
with LTO? Otherwise effects from inlining are limited to one compilation unit,
except for functions defined in headers. But for those, the kernel
uses 'static inline' anyway, so the patch wouldn't change anything.

If the original issue is that inlining duplicates code, wouldn't it be better
solved by a switch that instructs inlining heuristics to inline as if for -Os,
without enabling -Os for other passes?

Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jan Hubicka]

> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law  wrote:
> > On 9/26/18 7:38 AM, Jason Merrill wrote:
> >> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
> >>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
> >>> what 'internal' would mean in this context.
> >>
> >> I mean internal linkage as in the C and C++ standards.
> 
> > Since this is primarily for kernel hot patching, I think we're looking
> > to restrict inlining to functions that have visibility limited to a
> > compilation unit.
> 
> Right, which is internal linkage.
> 
> C11: "Within one translation unit, each declaration of an identifier
> with internal linkage denotes the same object or function."
> C++17: "When a name has internal linkage, the entity it denotes can be
> referred to by names from other scopes in the same translation unit."
> 
> Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
> richi suggested.

I am not quite sure if we can relate visibility flags we have at this stage
to visibility in source languge in very coherent way.  They change a lot with
LTO and we may want to make this option incompatible with LTO, but even w/o
we can turn function static that previously wasn't.

For example comdat that was cloned by IPA-SRA. See can_be_local_p and
comdat_can_be_unshared_p predicates.  Similar problem happens to clones created
by ipa-cp.

I guess we want to disable localization and cloning in this case as well.
I wonder what else.

Honza

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jason Merrill]

On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law  wrote:
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
>>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
>>> what 'internal' would mean in this context.
>>
>> I mean internal linkage as in the C and C++ standards.

> Since this is primarily for kernel hot patching, I think we're looking
> to restrict inlining to functions that have visibility limited to a
> compilation unit.

Right, which is internal linkage.

C11: "Within one translation unit, each declaration of an identifier
with internal linkage denotes the same object or function."
C++17: "When a name has internal linkage, the entity it denotes can be
referred to by names from other scopes in the same translation unit."

Or perhaps we want to say "not external linkage", i.e. !TREE_PUBLIC as
richi suggested.

Jason

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jeff Law]

On 9/26/18 7:38 AM, Jason Merrill wrote:
> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
>> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
>> what 'internal' would mean in this context.
> 
> I mean internal linkage as in the C and C++ standards.
Since this is primarily for kernel hot patching, I think we're looking
to restrict inlining to functions that have visibility limited to a
compilation unit.

Qing, can you confirm that either way?

Jeff

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

Alexander,

thanks for the questions.

Yes, we had some discussion on the questions you raised during the review of 
the initial patch back to 9/11/2018.
please take a look at those discussions at:

https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00549.html 

https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00787.html 


and let me know if those discussion still does not answer your questions.

Qing

> On Sep 26, 2018, at 6:12 AM, Alexander Monakov  wrote:
> 
> On Fri, 21 Sep 2018, Qing Zhao wrote:
>> +2018-09-20  Qing Zhao  
>> +
>> +* cif-code.def (FUNCTION_EXTERN): New CIFCODE.
>> +* common.opt (-finline-only-static): New option.
>> +* doc/invoke.texi: Document -finline-only-static.
>> +* ipa-inline.c (can_inline_edge_p): Control inlining based on
>> +function's linkage. 
> 
> Note, I am not a reviewer.
> 
> In my opinion, there's a problem with the patch that it looks like an ad-hoc,
> incomplete solution. You said you need this change to help with building
> livepatching-capable kernels, but it's not clear what exactly the issue with
> inlining non-static functions is. Can you describe how the workflow looks like
> so code duplication due to inlining static functions is not an issue, but
> inlining non-static functions is a problem? Does using existing
> -fno-inline-functions flag achieve something useful for your usecase?
> 
> Please always make it clear what problem the patch is intended to solve and 
> help
> reviewers see the connection between the problem and your solution. Look how 
> the
> "XY problem" effect applies partially in this situation.
> 
> https://en.wikipedia.org/wiki/XY_problem
> http://xyproblem.info/
> 
> Alexander

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jason Merrill]

On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener  wrote:
> IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
> what 'internal' would mean in this context.

I mean internal linkage as in the C and C++ standards.

Jason

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Richard Biener]

On Wed, 26 Sep 2018, Jason Merrill wrote:

> On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao  wrote:
> > Hi, this is the 4th version of the patch.
> >
> > mainly address Martin’s comments on some spelling issues.
> >
> > I have tested the patch on both x86 and aarch64, no issue.
> >
> > Okay for commit?
> >
> > thanks.
> >
> > Qing.
> >
> > gcc/ChangeLog
> >
> > +2018-09-20  Qing Zhao  
> > +
> > +   * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
> > +   * common.opt (-finline-only-static): New option.
> > +   * doc/invoke.texi: Document -finline-only-static.
> > +   * ipa-inline.c (can_inline_edge_p): Control inlining based on
> > +   function's linkage.
> 
> I would suggest "internal" rather than "static" in general.  So
> 
> +N_("function has external linkage when the user requests only"
> +   " inlining functions with internal linkage"))
> 
> +Inline functions only if they have internal linkage.
> 
> +@item -finline-only-internal
> +@opindex finline-only-internal
> +By default, GCC inlines functions without considering their linkage.
> +This flag guides the inliner to only inline functions with internal linkage.
> +This option has any effect only when inlining itself is turned on by the
> +-finline-functions or -finline-small-functions options.
> 
> This should also mention whether it applies to functions explicitly
> declared inline; I assume it does not.

IIRC he explicitely wanted 'static' not 'hidden' linkage.  Not sure
what 'internal' would mean in this context.

But then the implementation looks at callee->externally_visible which
matches hidden visibility... externally_visible is probably not
the very best thing to look at depending on what we intend to do.

What about 'static' functions with their address taken?

Note you shouldn't look at individual cgraph_node fields but use
some of the accessors with more well-constrained semantics.
Why are you not simply checking !TREE_PUBLIC?

Honza might be able to suggest one.

Richard.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Jason Merrill]

On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao  wrote:
> Hi, this is the 4th version of the patch.
>
> mainly address Martin’s comments on some spelling issues.
>
> I have tested the patch on both x86 and aarch64, no issue.
>
> Okay for commit?
>
> thanks.
>
> Qing.
>
> gcc/ChangeLog
>
> +2018-09-20  Qing Zhao  
> +
> +   * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
> +   * common.opt (-finline-only-static): New option.
> +   * doc/invoke.texi: Document -finline-only-static.
> +   * ipa-inline.c (can_inline_edge_p): Control inlining based on
> +   function's linkage.

I would suggest "internal" rather than "static" in general.  So

+N_("function has external linkage when the user requests only"
+   " inlining functions with internal linkage"))

+Inline functions only if they have internal linkage.

+@item -finline-only-internal
+@opindex finline-only-internal
+By default, GCC inlines functions without considering their linkage.
+This flag guides the inliner to only inline functions with internal linkage.
+This option has any effect only when inlining itself is turned on by the
+-finline-functions or -finline-small-functions options.

This should also mention whether it applies to functions explicitly
declared inline; I assume it does not.

Jason

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Paolo Carlini]

Hi,

On 9/26/18 1:12 PM, Alexander Monakov wrote:

On Fri, 21 Sep 2018, Qing Zhao wrote:

+2018-09-20  Qing Zhao  
+
+   * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
+   * common.opt (-finline-only-static): New option.
+   * doc/invoke.texi: Document -finline-only-static.
+   * ipa-inline.c (can_inline_edge_p): Control inlining based on
+   function's linkage.

Note, I am not a reviewer.

In my opinion, there's a problem with the patch that it looks like an ad-hoc,
incomplete solution.


It is not. Actually, I don't understand why we are raising this sort of 
issue now, after so many messages, like, for example Jeff's, which 
should have fully clarified the rationale.


Paolo.

Re: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Alexander Monakov]

On Fri, 21 Sep 2018, Qing Zhao wrote:
> +2018-09-20  Qing Zhao  
> +
> + * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
> + * common.opt (-finline-only-static): New option.
> + * doc/invoke.texi: Document -finline-only-static.
> + * ipa-inline.c (can_inline_edge_p): Control inlining based on
> + function's linkage. 

Note, I am not a reviewer.

In my opinion, there's a problem with the patch that it looks like an ad-hoc,
incomplete solution. You said you need this change to help with building
livepatching-capable kernels, but it's not clear what exactly the issue with
inlining non-static functions is. Can you describe how the workflow looks like
so code duplication due to inlining static functions is not an issue, but
inlining non-static functions is a problem? Does using existing
-fno-inline-functions flag achieve something useful for your usecase?

Please always make it clear what problem the patch is intended to solve and help
reviewers see the connection between the problem and your solution. Look how the
"XY problem" effect applies partially in this situation.

https://en.wikipedia.org/wiki/XY_problem
http://xyproblem.info/

Alexander

PING: [PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

Hi, 

I’d like to ping the following patch.

https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01238.html 


Thanks.

Qing

[PATCH][Middle-end][Version 4]Add a new option to control inlining only on static functions

[Qing Zhao]

Hi, this is the 4th version of the patch.

mainly address Martin’s comments on some spelling issues.

I have tested the patch on both x86 and aarch64, no issue.

Okay for commit?

thanks.

Qing.

gcc/ChangeLog

+2018-09-20  Qing Zhao  
+
+   * cif-code.def (FUNCTION_EXTERN): New CIFCODE.
+   * common.opt (-finline-only-static): New option.
+   * doc/invoke.texi: Document -finline-only-static.
+   * ipa-inline.c (can_inline_edge_p): Control inlining based on
+   function's linkage. 

gcc/testsuite/ChangeLog

+2018-09-20  Qing Zhao  
+
+   * gcc.dg/inline_only_static.c: New test.
+



New-inline-only-static.patch
Description: Binary data