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

classic Classic list List threaded Threaded
100 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
Hi,

this is the 3rd version of the patch, the major change is to address Andrew’s concern on the documentation part.

I updated the documentation of this option as following:

'-finline-only-static'
     By default, GCC inlines functions without considering whether they
     are static or not.  This flag guides inliner to only inline static
     functions.  This option has any effect only when inlining itself is
     turned on by the -finline-functions or -finline-small-fiunctions.

     Off by default.

all other changes keep the same as version 2.

please take a look again. and let me know any comment and suggestion.

thanks.

Qing

gcc/ChangeLog

+2018-09-18  Qing Zhao  <[hidden email]>
+
+ * 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 visibility.

gcc/testsuite/ChangeLog

+2018-09-18  Qing Zhao  <[hidden email]>
+
+ * gcc.dg/inline_only_static.c: New test.
+


New-inline-only-static.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
On 09/18/2018 12:58 PM, Qing Zhao wrote:

> Hi,
>
> this is the 3rd version of the patch, the major change is to address Andrew’s concern on the documentation part.
>
> I updated the documentation of this option as following:
>
> '-finline-only-static'
>      By default, GCC inlines functions without considering whether they
>      are static or not.  This flag guides inliner to only inline static
>      functions.  This option has any effect only when inlining itself is
>      turned on by the -finline-functions or -finline-small-fiunctions.
>
>      Off by default.
>
> all other changes keep the same as version 2.
>
> please take a look again. and let me know any comment and suggestion.

Just a few minor spelling issues:

>
> thanks.
>
> Qing
>
> gcc/ChangeLog
>
> +2018-09-18  Qing Zhao  <[hidden email]>
> +
> + * 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 visibility.

Probably "linkage" would be a more fitting term here.

>
> gcc/testsuite/ChangeLog
>
> +2018-09-18  Qing Zhao  <[hidden email]>
> +
> + * gcc.dg/inline_only_static.c: New test.
> +


diff --git a/gcc/cif-code.def b/gcc/cif-code.def
index 19a7621..64b2b1a 100644
--- a/gcc/cif-code.def
+++ b/gcc/cif-code.def
@@ -132,6 +132,12 @@
  DEFCIFCODE(USES_COMDAT_LOCAL, CIF_FINAL_ERROR,
  DEFCIFCODE(ATTRIBUTE_MISMATCH, CIF_FINAL_ERROR,
    N_("function attribute mismatch"))

+/* We can't inline because the user requests only inlining static
function
+   but the function is external visible.  */

I suspect you meant: "only static functions" (plural) and
"the function has external linkage" (as defined in the C and
C++ standards).

+DEFCIFCODE(FUNCTION_EXTERN, CIF_FINAL_ERROR,
+   N_("function is external visible when the user requests only"
+      " inlining static"))
+

Here as well: either "function has external linkage" or "function
is extern."

=======================================
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ec12711..b6b0db5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@ -8066,6 +8067,15 @@
  having large chains of nested wrapper functions.

  Enabled by default.

+@item -finline-only-static
+@opindex finline-only-static
+By default, GCC inlines functions without considering whether they are
static
+or not. This flag guides inliner to only inline static functions.

Guides "the inliner" (missing article).

+This option has any effect only when inlining itself is turned on by the
+-finline-functions or -finline-small-fiunctions.

"by the -f... options."  (Missing "options") and
-finline-small-functions (note the spelling of functions).

+
+Off by default.

I think the customary way to word it is: "Disabled by default."
or "The finline-only-static option/flag is disabled/off by default"

Martin
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
thanks, Martin.

> On Sep 18, 2018, at 5:26 PM, Martin Sebor <[hidden email]> wrote:
>>
>> gcc/ChangeLog
>>
>> +2018-09-18  Qing Zhao  <[hidden email] <mailto:[hidden email]>>
>> +
>> + * 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 visibility.
>
> Probably "linkage" would be a more fitting term here.
Okay.

>
>>
>> gcc/testsuite/ChangeLog
>>
>> +2018-09-18  Qing Zhao  <[hidden email] <mailto:[hidden email]>>
>> +
>> + * gcc.dg/inline_only_static.c: New test.
>> +
>
>
> diff --git a/gcc/cif-code.def b/gcc/cif-code.def
> index 19a7621..64b2b1a 100644
> --- a/gcc/cif-code.def
> +++ b/gcc/cif-code.def
> @@ -132,6 +132,12 @@
> DEFCIFCODE(USES_COMDAT_LOCAL, CIF_FINAL_ERROR,
> DEFCIFCODE(ATTRIBUTE_MISMATCH, CIF_FINAL_ERROR,
>   N_("function attribute mismatch"))
>
> +/* We can't inline because the user requests only inlining static function
> +   but the function is external visible.  */
>
> I suspect you meant: "only static functions" (plural) and
> "the function has external linkage" (as defined in the C and
> C++ standards).
Okay.

>
> +DEFCIFCODE(FUNCTION_EXTERN, CIF_FINAL_ERROR,
> +   N_("function is external visible when the user requests only"
> +      " inlining static"))
> +
>
> Here as well: either "function has external linkage" or "function
> is extern.”
Okay.

>
> =======================================
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index ec12711..b6b0db5 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @ -8066,6 +8067,15 @@
> having large chains of nested wrapper functions.
>
> Enabled by default.
>
> +@item -finline-only-static
> +@opindex finline-only-static
> +By default, GCC inlines functions without considering whether they are static
> +or not. This flag guides inliner to only inline static functions.
>
> Guides "the inliner" (missing article).
Okay.

>
> +This option has any effect only when inlining itself is turned on by the
> +-finline-functions or -finline-small-fiunctions.
>
> "by the -f... options."  (Missing "options") and
> -finline-small-functions (note the spelling of functions).

Okay.
>
> +
> +Off by default.
>
> I think the customary way to word it is: "Disabled by default."
> or "The finline-only-static option/flag is disabled/off by default”

Okay.

Qing
>
> Martin

Reply | Threaded
Open this post in threaded view
|

[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  <[hidden email]>
+
+ * 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  <[hidden email]>
+
+ * gcc.dg/inline_only_static.c: New test.
+


New-inline-only-static.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
Reply | Threaded
Open this post in threaded view
|

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

Alexander Monakov-4
In reply to this post by Qing Zhao
On Fri, 21 Sep 2018, Qing Zhao wrote:
> +2018-09-20  Qing Zhao  <[hidden email]>
> +
> + * 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
Reply | Threaded
Open this post in threaded view
|

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

Paolo Carlini-3
Hi,

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

> On Fri, 21 Sep 2018, Qing Zhao wrote:
>> +2018-09-20  Qing Zhao  <[hidden email]>
>> +
>> + * 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.

Reply | Threaded
Open this post in threaded view
|

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

Jason Merrill
In reply to this post by Qing Zhao
On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao <[hidden email]> 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  <[hidden email]>
> +
> +       * 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
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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  <[hidden email]>
> > +
> > +       * 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.
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
In reply to this post by Alexander Monakov-4
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/msg00549.html>
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00787.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 <[hidden email]> wrote:
>
> On Fri, 21 Sep 2018, Qing Zhao wrote:
>> +2018-09-20  Qing Zhao  <[hidden email]>
>> +
>> + * 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

Reply | Threaded
Open this post in threaded view
|

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

Jeff Law
In reply to this post by Jason Merrill
On 9/26/18 7:38 AM, Jason Merrill wrote:
> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Jan Hubicka-2
> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law <[hidden email]> wrote:
> > On 9/26/18 7:38 AM, Jason Merrill wrote:
> >> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Alexander Monakov-4
In reply to this post by Qing Zhao
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/msg00549.html>
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00787.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
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
In reply to this post by Richard Biener

> On Sep 26, 2018, at 8:24 AM, Richard Biener <[hidden email]> wrote:
>
> On Wed, 26 Sep 2018, Jason Merrill wrote:
>
>> On Fri, Sep 21, 2018 at 11:12 AM, Qing Zhao <[hidden email]> 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  <[hidden email]>
>>> +
>>> +       * 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.

Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
In reply to this post by Jeff Law

> On Sep 26, 2018, at 9:45 AM, Jeff Law <[hidden email]> wrote:
>
> On 9/26/18 7:38 AM, Jason Merrill wrote:
>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Jan Hubicka-2
In reply to this post by Qing Zhao
> >  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
Reply | Threaded
Open this post in threaded view
|

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

Qing Zhao
In reply to this post by Jan Hubicka-2

> On Sep 26, 2018, at 10:06 AM, Jan Hubicka <[hidden email]> wrote:
>
>> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law <[hidden email]> wrote:
>>> On 9/26/18 7:38 AM, Jason Merrill wrote:
>>>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Jan Hubicka-2
>
> > On Sep 26, 2018, at 10:06 AM, Jan Hubicka <[hidden email]> wrote:
> >
> >> On Wed, Sep 26, 2018 at 10:45 AM, Jeff Law <[hidden email]> wrote:
> >>> On 9/26/18 7:38 AM, Jason Merrill wrote:
> >>>> On Wed, Sep 26, 2018 at 9:24 AM, Richard Biener <[hidden email]> 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
>
12345