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

[Qing Zhao <qing dot zhao at oracle dot com>]

> On Sep 26, 2018, at 8:24 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Wed, 26 Sep 2018, Jason Merrill wrote:
> 
>> On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao <qing.zhao@oracle.com> 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  <qing.zhao@oracle.com>
>>> +
>>> +       * 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.