[RFC] Come up with -flive-patching master option.

Message ID	e68f7f65-5e71-3f2b-4f47-fcbb90395af4@suse.cz
State	New
Headers	show Return-Path: <gcc-patches-return-489532-incoming=patchwork.ozlabs.org@gcc.gnu.org> DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=a7gSj/R8eraqPYLwY wmzkghwr6ggEpq1tUgiz3kG1YARW60tqWvM9+eRFWykRtXbt6GpC9KNoYJ3yvKDL tu/BKHGYIeLMvSv+JTztnMHwQnZZUvnO/5Nx7Nd9IEorAqNUUpewdCMXhRko6R/F eZXwATtQP/0MYCfaUXxh47ZGQU= Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org Subject: [PATCH][RFC] Come up with -flive-patching master option. To: Jan Hubicka <hubicka@ucw.cz> Cc: Qing Zhao <qing.zhao@oracle.com>, Martin Jambor <mjambor@suse.cz>, live-patching@vger.kernel.org, gcc Patches <gcc-patches@gcc.gnu.org> References: <1a023bdc-28a6-eb41-b449-4d096f12064f@suse.cz> <048D9997-B7AF-444A-BF7E-79944DE8F174@oracle.com> <add5773e-0cea-58c9-b458-bf256f81d057@suse.cz> <3E37D3A8-2D19-41C2-BA8A-8F0EFA1B4D5C@oracle.com> <10a54034-279b-a406-8466-55558effbf24@suse.cz> <20181003090457.GJ57692@kam.mff.cuni.cz> <54a75932-201b-671c-0a63-d1a5d8d7b562@suse.cz> <90c91045-cb9d-0bd2-fad3-d16426ceede6@suse.cz> <20181105095135.j3mnzox6rkktkoto@kam.mff.cuni.cz> <629b20d2-7b0b-9342-a64a-d12eb6e6a6b7@suse.cz> <20181108145909.5eh5ccen7a7elspe@kam.mff.cuni.cz> From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Message-ID: <e68f7f65-5e71-3f2b-4f47-fcbb90395af4@suse.cz> Date: Fri, 9 Nov 2018 16:33:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181108145909.5eh5ccen7a7elspe@kam.mff.cuni.cz> Content-Type: multipart/mixed; boundary="------------2A65AB06AA73D314649ED118"
Series	[RFC] Come up with -flive-patching master option. \| expand [RFC] Come up with -flive-patching master option.

Martin Liška Nov. 9, 2018, 3:33 p.m. UTC

Hi.

After I added 2 new options, I would like to include a new master option.
It's minimal version which only disables optimizations that we are aware of
and can potentially cause problems for live-patching.

Martin

Qing Zhao Nov. 9, 2018, 5:43 p.m. UTC | #1

Hi, Martin,

thanks a lot for the previous two new options for live-patching. 

I have two more questions below:

1. do we still need new options to disable the following:
   A. unreachable code/variable removal? 
   B. Visibility changes with -flto and/or -fwhole-program?

2. for this new patch, could you please explain a little bit more on the problem?

Thanks.

Qing

> On Nov 9, 2018, at 9:33 AM, Martin Liška <mliska@suse.cz> wrote:
> 
> Hi.
> 
> After I added 2 new options, I would like to include a new master option.
> It's minimal version which only disables optimizations that we are aware of
> and can potentially cause problems for live-patching.
> 
> Martin
> <0001-Come-up-with-fvectorized-functions.patch>

Bernhard Reutner-Fischer Nov. 9, 2018, 6:38 p.m. UTC | #2

On 9 November 2018 16:33:22 CET, "Martin Liška" <mliska@suse.cz> wrote:
>Hi.
>
>After I added 2 new options, I would like to include a new master
>option.
>It's minimal version which only disables optimizations that we are
>aware of
>and can potentially cause problems for live-patching.

I think you attached the wrong patch, AFAICS you attached the gfortran vectorized_builtins patch..

thanks,

Martin Liška Nov. 10, 2018, 8:48 a.m. UTC | #3

Hi.

Sorry for attaching a wrong patch.

Martin
From 7d3887b1b24901eca69614e601b6e8f36e5c86eb Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 9 Nov 2018 16:28:07 +0100
Subject: [PATCH] Come up with -flive-patching master option.

gcc/ChangeLog:

2018-11-09  Martin Liska  <mliska@suse.cz>

	* common.opt: Add -flive-patching option.
	* opts.c (disable_live_patching_related_optimizations): New
	function where we disable all optimizations that are potentially
	dangerous for live patching.
	(common_handle_option): Call the function.
---
 gcc/common.opt |  4 ++++
 gcc/opts.c     | 31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/gcc/common.opt b/gcc/common.opt
index 98e8eb03ef3..475b667f26b 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2156,6 +2156,10 @@ flive-range-shrinkage
 Common Report Var(flag_live_range_shrinkage) Init(0) Optimization
 Relief of register pressure through live range shrinkage.
 
+flive-patching
+Common Report Var(flag_live_patching) Init(0) Optimization
+Perform only safe IPA transformations for live patching.
+
 frename-registers
 Common Report Var(flag_rename_registers) Init(2) Optimization
 Perform a register renaming optimization pass.
diff --git a/gcc/opts.c b/gcc/opts.c
index e21967ba84d..dd0c24ac2f9 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1548,6 +1548,33 @@ enable_fdo_optimizations (struct gcc_options *opts,
     opts->x_flag_tree_loop_distribute_patterns = value;
 }
 
+/* Disable IPA optimization that are not safe for live patching.  */
+
+static void
+disable_live_patching_related_optimizations (gcc_options *opts,
+					     gcc_options *opts_set)
+{
+  if (!opts_set->x_flag_ipa_icf_functions)
+    opts->x_flag_ipa_icf_functions = 0;
+
+  if (!opts_set->x_flag_ipa_icf)
+      opts->x_flag_ipa_icf = 0;
+  if (!opts_set->x_flag_ipa_icf_variables)
+      opts->x_flag_ipa_icf_variables = 0;
+  if (!opts_set->x_flag_ipa_pure_const)
+      opts->x_flag_ipa_pure_const = 0;
+  if (!opts_set->x_flag_ipa_pta)
+      opts->x_flag_ipa_pta = 0;
+  if (!opts_set->x_flag_ipa_reference)
+      opts->x_flag_ipa_reference = 0;
+  if (!opts_set->x_flag_ipa_reference_addressable)
+      opts->x_flag_ipa_reference_addressable = 0;
+  if (!opts_set->x_flag_ipa_stack_alignment)
+      opts->x_flag_ipa_stack_alignment = 0;
+  if (!opts_set->x_flag_ipa_ra)
+      opts->x_flag_ipa_ra = 0;
+}
+
 /* -f{,no-}sanitize{,-recover}= suboptions.  */
 const struct sanitizer_opts_s sanitizer_opts[] =
 {
@@ -2266,6 +2293,10 @@ common_handle_option (struct gcc_options *opts,
 	(&opts->x_flag_instrument_functions_exclude_files, arg);
       break;
 
+    case OPT_flive_patching:
+      if (value)
+	disable_live_patching_related_optimizations (opts, opts_set);
+
     case OPT_fmessage_length_:
       pp_set_line_maximum_length (dc->printer, value);
       diagnostic_set_caret_max_width (dc, value);

Martin Liška Nov. 10, 2018, 8:51 a.m. UTC | #4

On 11/9/18 6:43 PM, Qing Zhao wrote:
> Hi, Martin,
> 
> thanks a lot for the previous two new options for live-patching. 
> 
> 
> I have two more questions below:

Hello.

> 
> 1. do we still need new options to disable the following:
>    A. unreachable code/variable removal? 

I hope it's guarded with newly added option -fipa-reference-addressable. Correct me
if I'm wrong.

>    B. Visibility changes with -flto and/or -fwhole-program?

The options are not used in linux kernel, thus I didn't consider.

> 
> 2. for this new patch, could you please explain a little bit more on the problem?

We want to enable a single option that will disable all possible (and future) optimizations
that influence live patching.

Martin

> 
> Thanks.
> 
> Qing
> 
>> On Nov 9, 2018, at 9:33 AM, Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> After I added 2 new options, I would like to include a new master option.
>> It's minimal version which only disables optimizations that we are aware of
>> and can potentially cause problems for live-patching.
>>
>> Martin
>> <0001-Come-up-with-fvectorized-functions.patch>
>

Qing Zhao Nov. 12, 2018, 2:28 a.m. UTC | #5

Hi,

> On Nov 10, 2018, at 2:51 AM, Martin Liška <mliska@suse.cz> wrote:
> 
> On 11/9/18 6:43 PM, Qing Zhao wrote:
>> Hi, Martin,
>> 
>> thanks a lot for the previous two new options for live-patching. 
>> 
>> 
>> I have two more questions below:
> 
> Hello.
> 
>> 
>> 1. do we still need new options to disable the following:
>>   A. unreachable code/variable removal? 
> 
> I hope it's guarded with newly added option -fipa-reference-addressable. Correct me
> if I'm wrong.

The followings are some previous discussions on this:

“
>>> 
>>> We perform discovery of functions/variables with no address taken and
>>> optimizations that are not valid otherwise such as duplicating them
>>> or doing skipping them for alias analysis (no flag to disable)
>> 
>> Can you be please more verbose here? What optimizations do you mean?

See ipa_discover_readonly_nonaddressable_vars. If addressable bit is
cleared we start analyzing uses of the variable via ipa_reference or so.
If writeonly bit is set, we start removing writes to the variable and if
readonly bit is set we skip any analysis about whether vairable changed.
“

the new -fipa-reference-addressable is to control the above,  seems not the unreachable code/variable removal?

> 
>>   B. Visibility changes with -flto and/or -fwhole-program?
> 
> The options are not used in linux kernel, thus I didn't consider.

Okay, I see.

> 
>> 
>> 2. for this new patch, could you please explain a little bit more on the problem?
> 
> We want to enable a single option that will disable all possible (and future) optimizations
> that influence live patching.

Okay, I see.

I am also working on a similar option as yours, but make the -flive-patching as two level control:

+flive-patching
+Common RejectNegative Alias(flive-patching=,inline-clone)
+
+flive-patching=
+Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE)
+-flive-patching=[inline-only-static|inline-clone]      Control optimizations to provide a safe comp for live-patching purpose.

the implementation for -flive-patching=inline-clone (the default) is exactly as yours,  the new level -flive-patching=inline-only-static
is to only enable inlining of static function for live patching, which is important for multiple-processes live patching to control memory
consumption. 

(please see my 2nd version of the -flive-patching proposal).

I will send out my complete patch in another email.

thanks.

Qing

Martin Liška Nov. 12, 2018, 8:53 a.m. UTC | #6

On 11/12/18 3:28 AM, Qing Zhao wrote:
> Hi,
> 
> 
>> On Nov 10, 2018, at 2:51 AM, Martin Liška <mliska@suse.cz> wrote:
>>
>> On 11/9/18 6:43 PM, Qing Zhao wrote:
>>> Hi, Martin,
>>>
>>> thanks a lot for the previous two new options for live-patching. 
>>>
>>>
>>> I have two more questions below:
>>
>> Hello.
>>
>>>
>>> 1. do we still need new options to disable the following:
>>>   A. unreachable code/variable removal? 
>>
>> I hope it's guarded with newly added option -fipa-reference-addressable. Correct me
>> if I'm wrong.
> 
> The followings are some previous discussions on this:
> 
> “
>>>>
>>>> We perform discovery of functions/variables with no address taken and
>>>> optimizations that are not valid otherwise such as duplicating them
>>>> or doing skipping them for alias analysis (no flag to disable)
>>>
>>> Can you be please more verbose here? What optimizations do you mean?
> 
> See ipa_discover_readonly_nonaddressable_vars. If addressable bit is
> cleared we start analyzing uses of the variable via ipa_reference or so.
> If writeonly bit is set, we start removing writes to the variable and if
> readonly bit is set we skip any analysis about whether vairable changed.
> “
> 
> the new -fipa-reference-addressable is to control the above,  seems not the unreachable code/variable removal?
> 
>>
>>>   B. Visibility changes with -flto and/or -fwhole-program?
>>
>> The options are not used in linux kernel, thus I didn't consider.
> 
> Okay, I see.
> 
>>
>>>
>>> 2. for this new patch, could you please explain a little bit more on the problem?
>>
>> We want to enable a single option that will disable all possible (and future) optimizations
>> that influence live patching.
> 
> Okay, I see.
> 
> I am also working on a similar option as yours, but make the -flive-patching as two level control:
> 
> +flive-patching
> +Common RejectNegative Alias(flive-patching=,inline-clone)
> +
> +flive-patching=
> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE)
> +-flive-patching=[inline-only-static|inline-clone]      Control optimizations to provide a safe comp for live-patching purpose.
> 
> the implementation for -flive-patching=inline-clone (the default) is exactly as yours,  the new level -flive-patching=inline-only-static
> is to only enable inlining of static function for live patching, which is important for multiple-processes live patching to control memory
> consumption. 
> 
> (please see my 2nd version of the -flive-patching proposal).
> 
> I will send out my complete patch in another email.

Hi, sure, works for me. Let's make 2 level option.

Martin

> 
> thanks.
> 
> Qing
> 
>

Martin Liška Nov. 12, 2018, 9:29 a.m. UTC | #7

On 11/10/18 6:03 PM, Jan Hubicka wrote:
>> On 11/9/18 6:43 PM, Qing Zhao wrote:
>>> Hi, Martin,
>>>
>>> thanks a lot for the previous two new options for live-patching. 
>>>
>>>
>>> I have two more questions below:
>>
>> Hello.
>>
>>>
>>> 1. do we still need new options to disable the following:
>>>    A. unreachable code/variable removal? 
>>
>> I hope it's guarded with newly added option -fipa-reference-addressable. Correct me
>> if I'm wrong.
> 
> No, unreachable code removal is still independent (we track all
> references and can remove variable with address taken)
> If you really want to keep all the symbols, you probably can mark
> everything as force_output like flag_keep_inline functions is
> implemented.  I am not sure how practical it would be though.

I see, that's probably not practical to start using a function/variable that
wasn't used before a live patch.

Martin

> 
> Honza
>>
>>>    B. Visibility changes with -flto and/or -fwhole-program?
>>
>> The options are not used in linux kernel, thus I didn't consider.
>>
>>>
>>> 2. for this new patch, could you please explain a little bit more on the problem?
>>
>> We want to enable a single option that will disable all possible (and future) optimizations
>> that influence live patching.
>>
>> Martin
>>
>>>
>>> Thanks.
>>>
>>> Qing
>>>
>>>> On Nov 9, 2018, at 9:33 AM, Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hi.
>>>>
>>>> After I added 2 new options, I would like to include a new master option.
>>>> It's minimal version which only disables optimizations that we are aware of
>>>> and can potentially cause problems for live-patching.
>>>>
>>>> Martin
>>>> <0001-Come-up-with-fvectorized-functions.patch>
>>>
>>

Qing Zhao Nov. 12, 2018, 10:29 p.m. UTC | #8

> On Nov 12, 2018, at 2:53 AM, Martin Liška <mliska@suse.cz> wrote:
> 
>> 
>> Okay, I see.
>> 
>> I am also working on a similar option as yours, but make the -flive-patching as two level control:
>> 
>> +flive-patching
>> +Common RejectNegative Alias(flive-patching=,inline-clone)
>> +
>> +flive-patching=
>> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE)
>> +-flive-patching=[inline-only-static|inline-clone]      Control optimizations to provide a safe comp for live-patching purpose.
>> 
>> the implementation for -flive-patching=inline-clone (the default) is exactly as yours,  the new level -flive-patching=inline-only-static
>> is to only enable inlining of static function for live patching, which is important for multiple-processes live patching to control memory
>> consumption. 
>> 
>> (please see my 2nd version of the -flive-patching proposal).
>> 
>> I will send out my complete patch in another email.
> 
> Hi, sure, works for me. Let's make 2 level option.

thank you.

I will send the patch tomorrow.

Qing
> 
> Martin

Qing Zhao Nov. 13, 2018, 5:48 p.m. UTC | #9

Hi,


Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.

'-flive-patching=LEVEL'
     Control GCC's optimizations to provide a safe compilation for
     live-patching.  Provides multiple-level control on how many of the
     optimizations are enabled by users' request.  The LEVEL argument
     should be one of the following:

     'inline-only-static'

          Only enable inlining of static functions, disable all other
          ipa optimizations/analyses.  As a result, when patching a
          static routine, all its callers need to be patches as well.

     'inline-clone'

          Only enable inlining and all optimizations that internally
          create clone, for example, cloning, ipa-sra, partial inlining,
          etc.; disable all other ipa optimizations/analyses.  As a
          result, when patching a routine, all its callers and its
          clones' callers need to be patched as well.

     When -flive-patching specified without any value, the default value
     is "inline-clone".

     This flag is disabled by default.

let me know your comments and suggestions on the implementation.

thanks a lot.

Qing
> On Nov 12, 2018, at 4:29 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
> 
>> On Nov 12, 2018, at 2:53 AM, Martin Liška <mliska@suse.cz> wrote:
>> 
>>> 
>>> Okay, I see.
>>> 
>>> I am also working on a similar option as yours, but make the -flive-patching as two level control:
>>> 
>>> +flive-patching
>>> +Common RejectNegative Alias(flive-patching=,inline-clone)
>>> +
>>> +flive-patching=
>>> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE)
>>> +-flive-patching=[inline-only-static|inline-clone]      Control optimizations to provide a safe comp for live-patching purpose.
>>> 
>>> the implementation for -flive-patching=inline-clone (the default) is exactly as yours,  the new level -flive-patching=inline-only-static
>>> is to only enable inlining of static function for live patching, which is important for multiple-processes live patching to control memory
>>> consumption. 
>>> 
>>> (please see my 2nd version of the -flive-patching proposal).
>>> 
>>> I will send out my complete patch in another email.
>> 
>> Hi, sure, works for me. Let's make 2 level option.
> 
> thank you.
> 
> I will send the patch tomorrow.
> 
> Qing
>> 
>> Martin
>

Miroslav Benes Nov. 13, 2018, 7:18 p.m. UTC | #10

On Tue, 13 Nov 2018, Qing Zhao wrote:

> Hi,

Hi, 

> Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.
> 
> '-flive-patching=LEVEL'
>      Control GCC's optimizations to provide a safe compilation for
>      live-patching.  Provides multiple-level control on how many of the
>      optimizations are enabled by users' request.  The LEVEL argument
>      should be one of the following:
> 
>      'inline-only-static'
> 
>           Only enable inlining of static functions, disable all other
>           ipa optimizations/analyses.  As a result, when patching a
>           static routine, all its callers need to be patches as well.
> 
>      'inline-clone'
> 
>           Only enable inlining and all optimizations that internally
>           create clone, for example, cloning, ipa-sra, partial inlining,
>           etc.; disable all other ipa optimizations/analyses.  As a
>           result, when patching a routine, all its callers and its
>           clones' callers need to be patched as well.

Based on our previous discussion I assume that "clone" optimizations are 
safe (for LP) and the others are not. Anyway I'd welcome a note mentioning 
that disabled optimizations are dangerous for LP.

I know it may be the same for you, but it is not for me as a GCC user. 
"internally create clone" sounds very... well, internal. It does not 
describe the option much for ordinary user whow has no knowledge about GCC 
internals.

So could you rephrase it a bit, please?

>      When -flive-patching specified without any value, the default value
>      is "inline-clone".
> 
>      This flag is disabled by default.
> 
> let me know your comments and suggestions on the implementation.

I compared it to Martin's patch and ipa-icf-variables is not covered in 
yours (I may have missed something).

Thanks,
Miroslav

Qing Zhao Nov. 13, 2018, 9:16 p.m. UTC | #11

Hi,

> On Nov 13, 2018, at 1:18 PM, Miroslav Benes <mbenes@suse.cz> wrote:
> 
>> Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.
>> 
>> '-flive-patching=LEVEL'
>>     Control GCC's optimizations to provide a safe compilation for
>>     live-patching.  Provides multiple-level control on how many of the
>>     optimizations are enabled by users' request.  The LEVEL argument
>>     should be one of the following:
>> 
>>     'inline-only-static'
>> 
>>          Only enable inlining of static functions, disable all other
>>          ipa optimizations/analyses.  As a result, when patching a
>>          static routine, all its callers need to be patches as well.
>> 
>>     'inline-clone'
>> 
>>          Only enable inlining and all optimizations that internally
>>          create clone, for example, cloning, ipa-sra, partial inlining,
>>          etc.; disable all other ipa optimizations/analyses.  As a
>>          result, when patching a routine, all its callers and its
>>          clones' callers need to be patched as well.
> 
> Based on our previous discussion I assume that "clone" optimizations are 
> safe (for LP) and the others are not. Anyway I'd welcome a note mentioning 
> that disabled optimizations are dangerous for LP.

actually, I don’t think that those disabled optimizations are “dangerous” for live-patching. one of the major reasons we disable them
is because that currently the compiler does NOT provide a good way to compute the impacted function list for those optimizations.
therefore, we disable them at this time. 

many of them could be enabled too if the compiler can report the impacted function list accurately in the future.



> 
> I know it may be the same for you, but it is not for me as a GCC user. 
> "internally create clone" sounds very... well, internal. It does not 
> describe the option much for ordinary user whow has no knowledge about GCC 
> internals.
> 
> So could you rephrase it a bit, please?

I tried to make this clear. please see the following:

'-flive-patching=LEVEL'
     Control GCC's optimizations to provide a safe compilation for
     live-patching.

     If the compiler's optimization uses a function's body or
     information extracted from its body to optimize/change another
     function, the latter is called an impacted function of the former.
     If a function is patched, its impacted functions should be patched
     too.

     The impacted functions are decided by the compiler's
     interprocedural optimizations.  For example, inlining a function
     into its caller, cloning a function and changing its caller to call
     this new clone, or extracting a function's pureness/constness
     information to optimize its direct or indirect callers, etc.

     Usually, the more ipa optimizations enabled, the larger the number
     of impacted functions for each function.  In order to control the
     number of impacted functions and computed the list of impacted
     function easily, we provide control to partially enable ipa
     optimizations on two different levels.

     The LEVEL argument should be one of the following:

     'inline-only-static'

          Only enable inlining of static functions, disable all other
          interprocedural optimizations/analyses.  As a result, when
          patching a static routine, all its callers need to be patches
          as well.

     'inline-clone'

          Only enable inlining and cloning optimizations, which includes
          inlining, cloning, interprocedural scalar replacement of
          aggregates and partial inlining.  Disable all other
          interprocedural optimizations/analyses.  As a result, when
          patching a routine, all its callers and its clones' callers
          need to be patched as well.

     When -flive-patching specified without any value, the default value
     is "inline-clone".

     This flag is disabled by default.


> 
>>     When -flive-patching specified without any value, the default value
>>     is "inline-clone".
>> 
>>     This flag is disabled by default.
>> 
>> let me know your comments and suggestions on the implementation.
> 
> I compared it to Martin's patch and ipa-icf-variables is not covered in 
> yours (I may have missed something).

Yes, you are right. I added this into my patch.

I am attaching the new patch here.
> 
> Thanks,
> Miroslav

Martin Liška Nov. 14, 2018, 3:03 p.m. UTC | #12

On 11/13/18 10:16 PM, Qing Zhao wrote:
> Hi,
> 
>> On Nov 13, 2018, at 1:18 PM, Miroslav Benes <mbenes@suse.cz> wrote:
>>
>>> Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.
>>>
>>> '-flive-patching=LEVEL'
>>>     Control GCC's optimizations to provide a safe compilation for
>>>     live-patching.  Provides multiple-level control on how many of the
>>>     optimizations are enabled by users' request.  The LEVEL argument
>>>     should be one of the following:
>>>
>>>     'inline-only-static'
>>>
>>>          Only enable inlining of static functions, disable all other
>>>          ipa optimizations/analyses.  As a result, when patching a
>>>          static routine, all its callers need to be patches as well.
>>>
>>>     'inline-clone'
>>>
>>>          Only enable inlining and all optimizations that internally
>>>          create clone, for example, cloning, ipa-sra, partial inlining,
>>>          etc.; disable all other ipa optimizations/analyses.  As a
>>>          result, when patching a routine, all its callers and its
>>>          clones' callers need to be patched as well.
>> Based on our previous discussion I assume that "clone" optimizations are 
>> safe (for LP) and the others are not. Anyway I'd welcome a note mentioning 
>> that disabled optimizations are dangerous for LP.
> actually, I don’t think that those disabled optimizations are “dangerous” for live-patching. one of the major reasons we disable them
> is because that currently the compiler does NOT provide a good way to compute the impacted function list for those optimizations.
> therefore, we disable them at this time. 
> 
> many of them could be enabled too if the compiler can report the impacted function list accurately in the future.
> 
> 
> 
>> I know it may be the same for you, but it is not for me as a GCC user. 
>> "internally create clone" sounds very... well, internal. It does not 
>> describe the option much for ordinary user whow has no knowledge about GCC 
>> internals.
>>
>> So could you rephrase it a bit, please?
> I tried to make this clear. please see the following:
> 
> '-flive-patching=LEVEL'
>      Control GCC's optimizations to provide a safe compilation for
>      live-patching.
> 
>      If the compiler's optimization uses a function's body or
>      information extracted from its body to optimize/change another
>      function, the latter is called an impacted function of the former.
>      If a function is patched, its impacted functions should be patched
>      too.
> 
>      The impacted functions are decided by the compiler's
>      interprocedural optimizations.  For example, inlining a function
>      into its caller, cloning a function and changing its caller to call
>      this new clone, or extracting a function's pureness/constness
>      information to optimize its direct or indirect callers, etc.
> 
>      Usually, the more ipa optimizations enabled, the larger the number
>      of impacted functions for each function.  In order to control the
>      number of impacted functions and computed the list of impacted
>      function easily, we provide control to partially enable ipa
>      optimizations on two different levels.
> 
>      The LEVEL argument should be one of the following:
> 
>      'inline-only-static'
> 
>           Only enable inlining of static functions, disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a static routine, all its callers need to be patches
>           as well.
> 
>      'inline-clone'
> 
>           Only enable inlining and cloning optimizations, which includes
>           inlining, cloning, interprocedural scalar replacement of
>           aggregates and partial inlining.  Disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a routine, all its callers and its clones' callers
>           need to be patched as well.
> 
>      When -flive-patching specified without any value, the default value
>      is "inline-clone".
> 
>      This flag is disabled by default.
> 
> 
>>>     When -flive-patching specified without any value, the default value
>>>     is "inline-clone".
>>>
>>>     This flag is disabled by default.
>>>
>>> let me know your comments and suggestions on the implementation.
>> I compared it to Martin's patch and ipa-icf-variables is not covered in 
>> yours (I may have missed something).
> Yes, you are right. I added this into my patch.
> 
> I am attaching the new patch here.

Hello.

Please use
git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py /tmp/patch

in order to address many formatting issues of the patch (skip the ones reported in common.opt).

> 
> 
> flive-patching.patch
> 
> From c0785675eb29599754aaf11c70901c12dd3ea821 Mon Sep 17 00:00:00 2001
> From: qing zhao <qing.zhao@oracle.com>
> Date: Tue, 13 Nov 2018 13:02:57 -0800
> Subject: [PATCH] Add -flive-patching to support live patching
> 
> ---
>  gcc/cif-code.def                       |  6 +++
>  gcc/common.opt                         | 18 +++++++++
>  gcc/doc/invoke.texi                    | 49 +++++++++++++++++++++++-
>  gcc/flag-types.h                       |  8 ++++
>  gcc/ipa-inline.c                       |  6 +++
>  gcc/opts.c                             | 68 ++++++++++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/live-patching-1.c | 22 +++++++++++
>  7 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/live-patching-1.c
> 
> diff --git a/gcc/cif-code.def b/gcc/cif-code.def
> index 19a7621..dffe15f 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 static functions 
> +   but the function has external linkage for live patching purpose.  */
> +DEFCIFCODE(EXTERN_LIVE_ONLY_STATIC, CIF_FINAL_ERROR,
> +	   N_("function has external linkage when the user requests only"
> +	      " inlining static for live patching"))
> +
>  /* We proved that the call is unreachable.  */
>  DEFCIFCODE(UNREACHABLE, CIF_FINAL_ERROR,
>  	   N_("unreachable"))
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 98e8eb0..346a361 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2152,6 +2152,24 @@ starts and when the destructor finishes.
>  flifetime-dse=
>  Common Joined RejectNegative UInteger Var(flag_lifetime_dse) Optimization IntegerRange(0, 2)
>  
> +flive-patching
> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
> +
> +flive-patching=
> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE) Optimization
> +-flive-patching=[inline-only-static|inline-clone]	Control ipa optimizations to provide a 

Please use 'IPA' instead of 'ipa', similarly in documentation.

> +safe compilation for live-patching. At the same time, provides multiple-level control on the  
> +enabled optimizations. 
> +
> +Enum
> +Name(live_patching_level) Type(enum live_patching_level) UnknownError(unknown Live-Patching Level %qs)
> +
> +EnumValue
> +Enum(live_patching_level) String(inline-only-static) Value(LIVE_INLINE_ONLY_STATIC)
> +
> +EnumValue
> +Enum(live_patching_level) String(inline-clone) Value(LIVE_INLINE_CLONE)
> +
>  flive-range-shrinkage
>  Common Report Var(flag_live_range_shrinkage) Init(0) Optimization
>  Relief of register pressure through live range shrinkage.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 4ea93a7..c473049 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}.
>  -fgcse-sm  -fhoist-adjacent-loads  -fif-conversion @gol
>  -fif-conversion2  -findirect-inlining @gol
>  -finline-functions  -finline-functions-called-once  -finline-limit=@var{n} @gol
> --finline-small-functions  -fipa-cp  -fipa-cp-clone @gol
> +-finline-small-functions -fipa-cp  -fipa-cp-clone @gol

This changes is probably not intended.

>  -fipa-bit-cp -fipa-vrp @gol
>  -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
>  -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
> +-flive-patching=@var{level} @gol
>  -fira-region=@var{region}  -fira-hoist-pressure @gol
>  -fira-loop-pressure  -fno-ira-share-save-slots @gol
>  -fno-ira-share-spill-slots @gol
> @@ -8982,6 +8983,52 @@ equivalences that are found only by GCC and equivalences found only by Gold.
>  
>  This flag is enabled by default at @option{-O2} and @option{-Os}.
>  
> +@item -flive-patching=@var{level}
> +@opindex flive-patching
> +Control GCC's optimizations to provide a safe compilation for live-patching.
> +
> +If the compiler's optimization uses a function's body or information extracted 
> +from its body to optimize/change another function, the latter is called an
> +impacted function of the former.  If a function is patched, its impacted
> +functions should be patched too.
> +
> +The impacted functions are decided by the compiler's interprocedural
> +optimizations. For example, inlining a function into its caller, cloning 
> +a function and changing its caller to call this new clone, or extracting
> +a function's pureness/constness information to optimize its direct or 
> +indirect callers, etc.
> +
> +Usually, the more ipa optimizations enabled, the larger the number of
> +impacted functions for each function. In order to control the number of
> +impacted functions and computed the list of impacted function easily, 
> +we provide control to partially enable ipa optimizations on two different 
> +levels.
> +
> +The @var{level} argument should be one of the following:
> +
> +@table @samp
> +
> +@item inline-only-static
> +
> +Only enable inlining of static functions, disable all other interprocedural 
> +optimizations/analyses. As a result, when patching a static routine,
> +all its callers need to be patches as well.
> +
> +@item inline-clone 
> +
> +Only enable inlining and cloning optimizations, which includes inlining, 
> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
> +Disable all other interprocedural optimizations/analyses.
> +As a result, when patching a routine, all its callers and its clones'
> +callers need to be patched as well.
> +
> +@end table
> +
> +When -flive-patching specified without any value, the default value
> +is "inline-clone".
> +
> +This flag is disabled by default.
> +
>  @item -fisolate-erroneous-paths-dereference
>  @opindex fisolate-erroneous-paths-dereference
>  Detect paths that trigger erroneous or undefined behavior due to
> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
> index 500f663..72e0f0f 100644
> --- a/gcc/flag-types.h
> +++ b/gcc/flag-types.h
> @@ -123,6 +123,14 @@ enum stack_reuse_level
>    SR_ALL
>  };
>  
> +/* The live patching level. */
> +enum live_patching_level
> +{
> +  LIVE_NONE = 0,
> +  LIVE_INLINE_ONLY_STATIC,
> +  LIVE_INLINE_CLONE

Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum
values are bit unclear.

> +};
> +
>  /* The algorithm used for basic block reordering.  */
>  enum reorder_blocks_algorithm
>  {
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index e04ede7..fadb437 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -377,6 +377,12 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
>        e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
>        inlinable = false;
>      }
> +  else if (callee->externally_visible 
> +	   && flag_live_patching == LIVE_INLINE_ONLY_STATIC) 
> +    {
> +      e->inline_failed = CIF_EXTERN_LIVE_ONLY_STATIC;
> +      inlinable = false;
> +    }
>    if (!inlinable && report)
>      report_inline_failed_reason (e);
>    return inlinable;
> diff --git a/gcc/opts.c b/gcc/opts.c
> index e21967b..281f9f4 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -691,6 +691,64 @@ default_options_optimization (struct gcc_options *opts,
>  			 lang_mask, handlers, loc, dc);
>  }
>  
> +/* Control ipa optimizations based on different live patching LEVEL */
> +static void
> +control_optimizations_for_live_patching (struct gcc_options *opts, 
> +					 struct gcc_options *opts_set, 
> +					 enum live_patching_level level)
> +{
> +  gcc_assert (level > LIVE_NONE);
> +
> +  switch (level)
> +    {
> +    case LIVE_INLINE_ONLY_STATIC:
> +      if (!opts_set->x_flag_ipa_cp_clone)
> +        opts->x_flag_ipa_cp_clone = 0;      
> +      if (!opts_set->x_flag_ipa_sra)
> +        opts->x_flag_ipa_sra = 0;
> +      if (!opts_set->x_flag_partial_inlining)
> +        opts->x_flag_partial_inlining = 0;
> +      if (!opts_set->x_flag_ipa_cp)
> +        opts->x_flag_ipa_cp = 0;      
> +      /* FALLTHROUGH */
> +    case LIVE_INLINE_CLONE:
> +      /* live patching should disable whole-program optimization.  */
> +      if (!opts_set->x_flag_whole_program)
> +        opts->x_flag_whole_program = 0;
> +      /* visibility change should be excluded by !flag_whole_program 
> +	 && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra 

You added sorry about LTO, maybe then !in_lto_p would be always true?

> +	 && !flag_partial_inlining.  */
> +      if (!opts_set->x_flag_ipa_pta)
> +        opts->x_flag_ipa_pta = 0;
> +      if (!opts_set->x_flag_ipa_reference)
> +        opts->x_flag_ipa_reference = 0;
> +      if (!opts_set->x_flag_ipa_ra)
> +        opts->x_flag_ipa_ra = 0;
> +      if (!opts_set->x_flag_ipa_icf)
> +        opts->x_flag_ipa_icf = 0;
> +      if (!opts_set->x_flag_ipa_icf_functions)
> +        opts->x_flag_ipa_icf_functions = 0;
> +      if (!opts_set->x_flag_ipa_icf_variables)
> +        opts->x_flag_ipa_icf_variables = 0;
> +      if (!opts_set->x_flag_ipa_bit_cp)
> +        opts->x_flag_ipa_bit_cp = 0;
> +      if (!opts_set->x_flag_ipa_vrp)
> +        opts->x_flag_ipa_vrp = 0;
> +      if (!opts_set->x_flag_ipa_pure_const)

Can you please explain why you included:
      if (!opts_set->x_flag_ipa_bit_cp)
        opts->x_flag_ipa_bit_cp = 0;
      if (!opts_set->x_flag_ipa_vrp)
        opts->x_flag_ipa_vrp = 0;

?

> +        opts->x_flag_ipa_pure_const = 0;
> +      /* unreachable code removal.  */
> +      /* discovery of functions/variables with no address taken.  */

^^^ these 2 comments looks misaligned.

> +      if (!opts_set->x_flag_ipa_reference_addressable)
> +        opts->x_flag_ipa_reference_addressable = 0;
> +      /* ipa stack alignment propagation.  */
> +      if (!opts_set->x_flag_ipa_stack_alignment)
> +        opts->x_flag_ipa_stack_alignment = 0;
> +      break;
> +    default: 
> +      gcc_assert (0);  
> +    }
> +}
> +
>  /* After all options at LOC have been read into OPTS and OPTS_SET,
>     finalize settings of those options and diagnose incompatible
>     combinations.  */
> @@ -1040,6 +1098,10 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>    if ((opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS) && opts->x_flag_tm)
>      sorry ("transactional memory is not supported with "
>  	   "%<-fsanitize=kernel-address%>");
> +
> +  /* Currently live patching is not support for LTO.  */
> +  if (opts->x_flag_live_patching && in_lto_p) 
> +    sorry ("live patching is not supported with LTO");
>  }
>  
>  #define LEFT_COLUMN	27
> @@ -2266,6 +2328,12 @@ common_handle_option (struct gcc_options *opts,
>  	(&opts->x_flag_instrument_functions_exclude_files, arg);
>        break;
>  
> +    case OPT_flive_patching_:
> +      if (value)
> +    	control_optimizations_for_live_patching (opts, opts_set,
> +						 opts->x_flag_live_patching);
> +      break;
> +
>      case OPT_fmessage_length_:
>        pp_set_line_maximum_length (dc->printer, value);
>        diagnostic_set_caret_max_width (dc, value);
> diff --git a/gcc/testsuite/gcc.dg/live-patching-1.c b/gcc/testsuite/gcc.dg/live-patching-1.c
> new file mode 100644
> index 0000000..6a1ea38
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline" } */
> +
> +extern int sum, n, m;
> +
> +int foo (int a)
> +{
> +  return a + n;
> +}
> +
> +static int bar (int b)
> +{
> +  return b * m;
> +}
> +
> +int main()
> +{
> +  sum = foo (m) + bar (n); 
> +  return 0;
> +}
> +
> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when the user requests only inlining static for live patching"  "inline" } } */
> -- 1.9.1

It would be also handy to test the newly added option.
Please add ChangeLog entry for the patch. Have you bootstrapped the patch and run test-suite?

Thanks,
Martin

> 
> 
> 
>> Thanks,
>> Miroslav
>

Qing Zhao Nov. 14, 2018, 5:54 p.m. UTC | #13

Hi, 


> On Nov 14, 2018, at 9:03 AM, Martin Liška <mliska@suse.cz> wrote:
> 
>>> 
>> Yes, you are right. I added this into my patch.
>> 
>> I am attaching the new patch here.
> 
> Hello.
> 
> Please use
> git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py /tmp/patch
> 
> in order to address many formatting issues of the patch (skip the ones reported in common.opt).

will do and fix the style issues.

> 
>> 
>> 
>> +flive-patching
>> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
>> +
>> +flive-patching=
>> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE) Optimization
>> +-flive-patching=[inline-only-static|inline-clone]	Control ipa optimizations to provide a 
> 
> Please use 'IPA' instead of 'ipa', similarly in documentation.
Okay.
> 
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}.
>> -fgcse-sm  -fhoist-adjacent-loads  -fif-conversion @gol
>> -fif-conversion2  -findirect-inlining @gol
>> -finline-functions  -finline-functions-called-once  -finline-limit=@var{n} @gol
>> --finline-small-functions  -fipa-cp  -fipa-cp-clone @gol
>> +-finline-small-functions -fipa-cp  -fipa-cp-clone @gol
> 
> This changes is probably not intended.
No. will delete it.

> 
>> 
>> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
>> index 500f663..72e0f0f 100644
>> --- a/gcc/flag-types.h
>> +++ b/gcc/flag-types.h
>> @@ -123,6 +123,14 @@ enum stack_reuse_level
>>   SR_ALL
>> };
>> 
>> +/* The live patching level. */
>> +enum live_patching_level
>> +{
>> +  LIVE_NONE = 0,
>> +  LIVE_INLINE_ONLY_STATIC,
>> +  LIVE_INLINE_CLONE
> 
> Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum
> values are bit unclear.

Okay.
> 
>> 
>> +      /* visibility change should be excluded by !flag_whole_program 
>> +	 && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra 
> 
> You added sorry about LTO, maybe then !in_lto_p would be always true?

Yes, since live-patching does not support LTO currently,  !in_lto_p is always TRUE. that’s the reason no need for a new flag to disable visibility change. 
> 
>> +	 && !flag_partial_inlining.  */
>> +      if (!opts_set->x_flag_ipa_pta)
>> +        opts->x_flag_ipa_pta = 0;
>> +      if (!opts_set->x_flag_ipa_reference)
>> +        opts->x_flag_ipa_reference = 0;
>> +      if (!opts_set->x_flag_ipa_ra)
>> +        opts->x_flag_ipa_ra = 0;
>> +      if (!opts_set->x_flag_ipa_icf)
>> +        opts->x_flag_ipa_icf = 0;
>> +      if (!opts_set->x_flag_ipa_icf_functions)
>> +        opts->x_flag_ipa_icf_functions = 0;
>> +      if (!opts_set->x_flag_ipa_icf_variables)
>> +        opts->x_flag_ipa_icf_variables = 0;
>> +      if (!opts_set->x_flag_ipa_bit_cp)
>> +        opts->x_flag_ipa_bit_cp = 0;
>> +      if (!opts_set->x_flag_ipa_vrp)
>> +        opts->x_flag_ipa_vrp = 0;
>> +      if (!opts_set->x_flag_ipa_pure_const)
> 
> Can you please explain why you included:
>      if (!opts_set->x_flag_ipa_bit_cp)
>        opts->x_flag_ipa_bit_cp = 0;
>      if (!opts_set->x_flag_ipa_vrp)
>        opts->x_flag_ipa_vrp = 0;

interprocedural bitwise constant propagation and interprocedural propagation of value ranges does not involve creating clones,
and the bitwise constant and value ranges info extracted during ipa-cp phase are used later by other optimizations. their effect on 
impact functions are not clear at this moment. that’s the reason I think we need to disable these two. 

Martin Jambor raised this issue during our previous discussion on 10/03/2018:
“
I was thinking a bit more about this and recalled that not all stuff
that IPA-CP nowadays does involves creating clones, so we have to add
also:
 - -fno-ipa-bit-cp, and
 - -fno-ipa-vrp.

These two just record info in the parameters of *callees* of functions
from which it extracted info, without any cloning involved.  Both were
introduced in GCC 7.

Thanks,

Martin
“
and I think he is right.



> ?
> 
>> +        opts->x_flag_ipa_pure_const = 0;
>> +      /* unreachable code removal.  */
>> +      /* discovery of functions/variables with no address taken.  */
> 
> ^^^ these 2 comments looks misaligned.

will fix them. 
> 
>> 
>> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c
>> @@ -0,0 +1,22 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline" } */
>> +
>> +extern int sum, n, m;
>> +
>> +int foo (int a)
>> +{
>> +  return a + n;
>> +}
>> +
>> +static int bar (int b)
>> +{
>> +  return b * m;
>> +}
>> +
>> +int main()
>> +{
>> +  sum = foo (m) + bar (n); 
>> +  return 0;
>> +}
>> +
>> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when the user requests only inlining static for live patching"  "inline" } } */
>> -- 1.9.1
> 
> It would be also handy to test the newly added option.

not sure how to test it? any suggestion?

> Please add ChangeLog entry for the patch.

Okay.

> Have you bootstrapped the patch and run test-suite?
did on aarch64. I will do it on x86_64 as well.

thanks.

Qing

Miroslav Benes Nov. 14, 2018, 10:05 p.m. UTC | #14

On Tue, 13 Nov 2018, Qing Zhao wrote:

> Hi,
> 
> > On Nov 13, 2018, at 1:18 PM, Miroslav Benes <mbenes@suse.cz> wrote:
> > 
> >> Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.
> >> 
> >> '-flive-patching=LEVEL'
> >>     Control GCC's optimizations to provide a safe compilation for
> >>     live-patching.  Provides multiple-level control on how many of the
> >>     optimizations are enabled by users' request.  The LEVEL argument
> >>     should be one of the following:
> >> 
> >>     'inline-only-static'
> >> 
> >>          Only enable inlining of static functions, disable all other
> >>          ipa optimizations/analyses.  As a result, when patching a
> >>          static routine, all its callers need to be patches as well.
> >> 
> >>     'inline-clone'
> >> 
> >>          Only enable inlining and all optimizations that internally
> >>          create clone, for example, cloning, ipa-sra, partial inlining,
> >>          etc.; disable all other ipa optimizations/analyses.  As a
> >>          result, when patching a routine, all its callers and its
> >>          clones' callers need to be patched as well.
> > 
> > Based on our previous discussion I assume that "clone" optimizations are 
> > safe (for LP) and the others are not. Anyway I'd welcome a note mentioning 
> > that disabled optimizations are dangerous for LP.
> 
> actually, I don’t think that those disabled optimizations are “dangerous” for live-patching. one of the major reasons we disable them
> is because that currently the compiler does NOT provide a good way to compute the impacted function list for those optimizations.
> therefore, we disable them at this time. 
> 
> many of them could be enabled too if the compiler can report the impacted function list accurately in the future.

Yes, you can formulate it like that. On the other hand, I (we) have always 
tried to keep a set of patched functions as small as possible. So some 
cost-benefit analysis would have to be done. However, that's another 
problem and we can discuss it later.

> > 
> > I know it may be the same for you, but it is not for me as a GCC user. 
> > "internally create clone" sounds very... well, internal. It does not 
> > describe the option much for ordinary user whow has no knowledge about GCC 
> > internals.
> > 
> > So could you rephrase it a bit, please?
> 
> I tried to make this clear. please see the following:
> 
> '-flive-patching=LEVEL'
>      Control GCC's optimizations to provide a safe compilation for
>      live-patching.
> 
>      If the compiler's optimization uses a function's body or
>      information extracted from its body to optimize/change another
>      function, the latter is called an impacted function of the former.
>      If a function is patched, its impacted functions should be patched
>      too.
> 
>      The impacted functions are decided by the compiler's
>      interprocedural optimizations.  For example, inlining a function
>      into its caller, cloning a function and changing its caller to call
>      this new clone, or extracting a function's pureness/constness
>      information to optimize its direct or indirect callers, etc.
> 
>      Usually, the more ipa optimizations enabled, the larger the number
>      of impacted functions for each function.  In order to control the
>      number of impacted functions and computed the list of impacted
>      function easily, we provide control to partially enable ipa
>      optimizations on two different levels.
> 
>      The LEVEL argument should be one of the following:
> 
>      'inline-only-static'
> 
>           Only enable inlining of static functions, disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a static routine, all its callers need to be patches
>           as well.
> 
>      'inline-clone'
> 
>           Only enable inlining and cloning optimizations, which includes
>           inlining, cloning, interprocedural scalar replacement of
>           aggregates and partial inlining.  Disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a routine, all its callers and its clones' callers
>           need to be patched as well.
> 
>      When -flive-patching specified without any value, the default value
>      is "inline-clone".
> 
>      This flag is disabled by default.

Sounds better. Thanks.

Miroslav

Martin Liška Nov. 15, 2018, 8:41 a.m. UTC | #15

On 11/14/18 6:54 PM, Qing Zhao wrote:
> Hi, 
> 
> 
>> On Nov 14, 2018, at 9:03 AM, Martin Liška <mliska@suse.cz> wrote:
>>
>>>>
>>> Yes, you are right. I added this into my patch.
>>>
>>> I am attaching the new patch here.
>>
>> Hello.
>>
>> Please use
>> git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py /tmp/patch
>>
>> in order to address many formatting issues of the patch (skip the ones reported in common.opt).
> 
> will do and fix the style issues.
> 
>>
>>>
>>>
>>> +flive-patching
>>> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
>>> +
>>> +flive-patching=
>>> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE) Optimization
>>> +-flive-patching=[inline-only-static|inline-clone]	Control ipa optimizations to provide a 
>>
>> Please use 'IPA' instead of 'ipa', similarly in documentation.
> Okay.
>>
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}.
>>> -fgcse-sm  -fhoist-adjacent-loads  -fif-conversion @gol
>>> -fif-conversion2  -findirect-inlining @gol
>>> -finline-functions  -finline-functions-called-once  -finline-limit=@var{n} @gol
>>> --finline-small-functions  -fipa-cp  -fipa-cp-clone @gol
>>> +-finline-small-functions -fipa-cp  -fipa-cp-clone @gol
>>
>> This changes is probably not intended.
> No. will delete it.
> 
>>
>>>
>>> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
>>> index 500f663..72e0f0f 100644
>>> --- a/gcc/flag-types.h
>>> +++ b/gcc/flag-types.h
>>> @@ -123,6 +123,14 @@ enum stack_reuse_level
>>>   SR_ALL
>>> };
>>>
>>> +/* The live patching level. */
>>> +enum live_patching_level
>>> +{
>>> +  LIVE_NONE = 0,
>>> +  LIVE_INLINE_ONLY_STATIC,
>>> +  LIVE_INLINE_CLONE
>>
>> Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum
>> values are bit unclear.
> 
> Okay.
>>
>>>
>>> +      /* visibility change should be excluded by !flag_whole_program 
>>> +	 && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra 
>>
>> You added sorry about LTO, maybe then !in_lto_p would be always true?
> 
> Yes, since live-patching does not support LTO currently,  !in_lto_p is always TRUE. that’s the reason no need for a new flag to disable visibility change. 

Hi.

Ok.

>>
>>> +	 && !flag_partial_inlining.  */
>>> +      if (!opts_set->x_flag_ipa_pta)
>>> +        opts->x_flag_ipa_pta = 0;
>>> +      if (!opts_set->x_flag_ipa_reference)
>>> +        opts->x_flag_ipa_reference = 0;
>>> +      if (!opts_set->x_flag_ipa_ra)
>>> +        opts->x_flag_ipa_ra = 0;
>>> +      if (!opts_set->x_flag_ipa_icf)
>>> +        opts->x_flag_ipa_icf = 0;
>>> +      if (!opts_set->x_flag_ipa_icf_functions)
>>> +        opts->x_flag_ipa_icf_functions = 0;
>>> +      if (!opts_set->x_flag_ipa_icf_variables)
>>> +        opts->x_flag_ipa_icf_variables = 0;
>>> +      if (!opts_set->x_flag_ipa_bit_cp)
>>> +        opts->x_flag_ipa_bit_cp = 0;
>>> +      if (!opts_set->x_flag_ipa_vrp)
>>> +        opts->x_flag_ipa_vrp = 0;
>>> +      if (!opts_set->x_flag_ipa_pure_const)
>>
>> Can you please explain why you included:
>>      if (!opts_set->x_flag_ipa_bit_cp)
>>        opts->x_flag_ipa_bit_cp = 0;
>>      if (!opts_set->x_flag_ipa_vrp)
>>        opts->x_flag_ipa_vrp = 0;
> 
> interprocedural bitwise constant propagation and interprocedural propagation of value ranges does not involve creating clones,
> and the bitwise constant and value ranges info extracted during ipa-cp phase are used later by other optimizations. their effect on 
> impact functions are not clear at this moment. that’s the reason I think we need to disable these two. 
> 
> Martin Jambor raised this issue during our previous discussion on 10/03/2018:
> “
> I was thinking a bit more about this and recalled that not all stuff
> that IPA-CP nowadays does involves creating clones, so we have to add
> also:
>  - -fno-ipa-bit-cp, and
>  - -fno-ipa-vrp.
> 
> These two just record info in the parameters of *callees* of functions
> from which it extracted info, without any cloning involved.  Both were
> introduced in GCC 7.
> 
> Thanks,
> 
> Martin
> “
> and I think he is right.

Great, thanks for clarification! I forgot about that.

And please can you mention in documentation which options are disabled with -flive-patching=*?
We usually do it, e.g. take a look at '-Os' option:

```
‘-Os’ disables the following optimization flags:
-falign-functions -falign-jumps -falign-loops
-falign-labels -freorder-blocks -freorder-blocks-algorithm=stc
-freorder-blocks-and-partition -fprefetch-loop-arrays
```

> 
> 
> 
>> ?
>>
>>> +        opts->x_flag_ipa_pure_const = 0;
>>> +      /* unreachable code removal.  */
>>> +      /* discovery of functions/variables with no address taken.  */
>>
>> ^^^ these 2 comments looks misaligned.
> 
> will fix them. 
>>
>>>
>>> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c
>>> @@ -0,0 +1,22 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline" } */
>>> +
>>> +extern int sum, n, m;
>>> +
>>> +int foo (int a)
>>> +{
>>> +  return a + n;
>>> +}
>>> +
>>> +static int bar (int b)
>>> +{
>>> +  return b * m;
>>> +}
>>> +
>>> +int main()
>>> +{
>>> +  sum = foo (m) + bar (n); 
>>> +  return 0;
>>> +}
>>> +
>>> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when the user requests only inlining static for live patching"  "inline" } } */
>>> -- 1.9.1
>>
>> It would be also handy to test the newly added option.
> 
> not sure how to test it? any suggestion?

I would just e.g. copy test for recently added
gcc.target/i386/ipa-stack-alignment.c and replace the
option with corresponding -flive-patching option.

> 
>> Please add ChangeLog entry for the patch.
> 
> Okay.

That will help you to set up a skeleton:
./contrib/mklog /tmp/patch > /tmp/changelog

> 
>> Have you bootstrapped the patch and run test-suite?
> did on aarch64. I will do it on x86_64 as well.

Good, please mentioned that when sending a patch next time.

One more nit, please use gcc_unreachable instead of gcc_assert (0).

Thanks for working on that,
Martin

> 
> thanks.
> 
> Qing
>

Qing Zhao Nov. 15, 2018, 3:41 p.m. UTC | #16

HI,

>>> 
>>> Can you please explain why you included:
>>>     if (!opts_set->x_flag_ipa_bit_cp)
>>>       opts->x_flag_ipa_bit_cp = 0;
>>>     if (!opts_set->x_flag_ipa_vrp)
>>>       opts->x_flag_ipa_vrp = 0;
>> 
>> interprocedural bitwise constant propagation and interprocedural propagation of value ranges does not involve creating clones,
>> and the bitwise constant and value ranges info extracted during ipa-cp phase are used later by other optimizations. their effect on 
>> impact functions are not clear at this moment. that’s the reason I think we need to disable these two. 
>> 
>> Martin Jambor raised this issue during our previous discussion on 10/03/2018:
>> “
>> I was thinking a bit more about this and recalled that not all stuff
>> that IPA-CP nowadays does involves creating clones, so we have to add
>> also:
>> - -fno-ipa-bit-cp, and
>> - -fno-ipa-vrp.
>> 
>> These two just record info in the parameters of *callees* of functions
>> from which it extracted info, without any cloning involved.  Both were
>> introduced in GCC 7.
>> 
>> Thanks,
>> 
>> Martin
>> “
>> and I think he is right.
> 
> Great, thanks for clarification! I forgot about that.
> 
> And please can you mention in documentation which options are disabled with -flive-patching=*?
> We usually do it, e.g. take a look at '-Os' option:
> 
> ```
> ‘-Os’ disables the following optimization flags:
> -falign-functions -falign-jumps -falign-loops
> -falign-labels -freorder-blocks -freorder-blocks-algorithm=stc
> -freorder-blocks-and-partition -fprefetch-loop-arrays
> ```
> 

will add this.

>> 
>> 
>>> 
>>> It would be also handy to test the newly added option.
>> 
>> not sure how to test it? any suggestion?
> 
> I would just e.g. copy test for recently added
> gcc.target/i386/ipa-stack-alignment.c and replace the
> option with corresponding -flive-patching option.

thanks for the suggestion. 
> 
>> 
>>> Please add ChangeLog entry for the patch.
>> 
>> Okay.
> 
> That will help you to set up a skeleton:
> ./contrib/mklog /tmp/patch > /tmp/changelog

thanks for the tip!

> 
>> 
>>> Have you bootstrapped the patch and run test-suite?
>> did on aarch64. I will do it on x86_64 as well.
> 
> Good, please mentioned that when sending a patch next time.
> 
> One more nit, please use gcc_unreachable instead of gcc_assert (0).

Okay.

thanks a lot for the help.

Qing

Qing Zhao Nov. 16, 2018, 1:36 a.m. UTC | #17

Hi,

this is the new version of the patch.

I have bootstrapped it on both aarch64 and x86,  no regression.

please take a look.

Okay for commit?

thanks.

Qing

==================================

gcc/ChangeLog:

2018-11-15  qing zhao  <qing.zhao@oracle.com>

	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
	* common.opt: Add -flive-patching flag.
	* doc/invoke.texi: Document -flive-patching.
	* flag-types.h (enum live_patching_level): New enum.
	* ipa-inline.c (can_inline_edge_p): Disable external functions from
	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
	* opts.c (control_optimizations_for_live_patching): New function.
	(finish_options): Make flag_live_patching incompatible with flag_lto.
	(common_handle_option): Handle flive-patching flag.

gcc/testsuite/ChangeLog:

2018-11-15  qing zhao  <qing.zhao@oracle.com>

	* gcc.dg/live-patching-1.c: New test.
	* gcc.dg/live-patching-2.c: New test.
	* gcc.dg/tree-ssa/writeonly-3.c: New test.
	* gcc.target/i386/ipa-stack-alignment-2.c: New test.

Martin Liška Nov. 16, 2018, 3:26 p.m. UTC | #18

On 11/16/18 2:36 AM, Qing Zhao wrote:
> Hi,
> 
> this is the new version of the patch.
> 
> I have bootstrapped it on both aarch64 and x86,  no regression.
> 
> please take a look.

Thanks for the updated version of the patch.
I have last small nits I see:

- gcc/common.opt: when running --help=common, the line is too long
- gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
- gcc/opts.c - do not mix spaces + tabs

With that I'm fine. But note that I'm not a maintainer :)

Thanks,
Martin

> 
> Okay for commit?
> 
> thanks.
> 
> Qing
> 
> ==================================
> 
> gcc/ChangeLog:
> 
> 2018-11-15  qing zhao  <qing.zhao@oracle.com>
> 
> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
> 	* common.opt: Add -flive-patching flag.
> 	* doc/invoke.texi: Document -flive-patching.
> 	* flag-types.h (enum live_patching_level): New enum.
> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
> 	* opts.c (control_optimizations_for_live_patching): New function.
> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
> 	(common_handle_option): Handle flive-patching flag.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-11-15  qing zhao  <qing.zhao@oracle.com>
> 
> 	* gcc.dg/live-patching-1.c: New test.
> 	* gcc.dg/live-patching-2.c: New test.
> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
>
From e44d8b88ac5fb712d5b5e7fdf2f2ad7f43b8ea09 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 16 Nov 2018 16:23:44 +0100
Subject: [PATCH] my fixes.

---
 gcc/common.opt      | 3 +--
 gcc/doc/invoke.texi | 8 ++++----
 gcc/opts.c          | 2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index 63cd6cc851d..35c24b8e8cf 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2187,8 +2187,7 @@ Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
 flive-patching=
 Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_PATCHING_NONE) Optimization
 -flive-patching=[inline-only-static|inline-clone]	Control IPA
-optimizations to provide a safe compilation for live-patching. At the same
-time, provides multiple-level control on the enabled IPA optimizations.
+optimizations to provide a safe compilation for live-patching.
 
 Enum
 Name(live_patching_level) Type(enum live_patching_level) UnknownError(unknown Live-Patching Level %qs)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 0fb67163490..9cccc4455fa 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9288,13 +9288,13 @@ impacted function of the former.  If a function is patched, its impacted
 functions should be patched too.
 
 The impacted functions are decided by the compiler's interprocedural
-optimizations. For example, inlining a function into its caller, cloning
+optimizations.  For example, inlining a function into its caller, cloning
 a function and changing its caller to call this new clone, or extracting
 a function's pureness/constness information to optimize its direct or
 indirect callers, etc.
 
 Usually, the more IPA optimizations enabled, the larger the number of
-impacted functions for each function. In order to control the number of
+impacted functions for each function.  In order to control the number of
 impacted functions and computed the list of impacted function easily,
 we provide control to partially enable IPA optimizations on two different
 levels.
@@ -9313,8 +9313,8 @@ callers need to be patched as well.
 @option{-flive-patching=inline-clone} disables the following optimization flags:
 @gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
 -fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
--fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
--fipa-stack-alignment}
+-fipa-bit-cp  -fipa-vrp  -fipa-pure-const @gol
+-fipa-reference-addressable -fipa-stack-alignment}
 
 @item inline-only-static
 
diff --git a/gcc/opts.c b/gcc/opts.c
index 570155816e3..0b5e89faeee 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2347,7 +2347,7 @@ common_handle_option (struct gcc_options *opts,
 
     case OPT_flive_patching_:
       if (value)
-    	control_optimizations_for_live_patching (opts, opts_set,
+	control_optimizations_for_live_patching (opts, opts_set,
 						 opts->x_flag_live_patching);
       break;

Jan Hubicka Nov. 16, 2018, 3:51 p.m. UTC | #19

> On 11/16/18 2:36 AM, Qing Zhao wrote:
> > Hi,
> > 
> > this is the new version of the patch.
> > 
> > I have bootstrapped it on both aarch64 and x86,  no regression.
> > 
> > please take a look.
> 
> Thanks for the updated version of the patch.
> I have last small nits I see:
> 
> - gcc/common.opt: when running --help=common, the line is too long
> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
> - gcc/opts.c - do not mix spaces + tabs
> 
> With that I'm fine. But note that I'm not a maintainer :)

I wonder what happens, when I pass like -flive-patching -fwhole-program
compared to -fwhole-program -flive-patching.
It seems to me that in first case we will end up with whole-program
optimization while in the second we won't.

I guess we should behave in a way that we disable the passes when
they are enabled implicitly (such as by -O2) but output an error when
once uses contradicting set of options, lie -flive-patching
-fwhole-program?

Honza
> 
> Thanks,
> Martin
> 
> > 
> > Okay for commit?
> > 
> > thanks.
> > 
> > Qing
> > 
> > ==================================
> > 
> > gcc/ChangeLog:
> > 
> > 2018-11-15  qing zhao  <qing.zhao@oracle.com>
> > 
> > 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
> > 	* common.opt: Add -flive-patching flag.
> > 	* doc/invoke.texi: Document -flive-patching.
> > 	* flag-types.h (enum live_patching_level): New enum.
> > 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
> > 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
> > 	* opts.c (control_optimizations_for_live_patching): New function.
> > 	(finish_options): Make flag_live_patching incompatible with flag_lto.
> > 	(common_handle_option): Handle flive-patching flag.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2018-11-15  qing zhao  <qing.zhao@oracle.com>
> > 
> > 	* gcc.dg/live-patching-1.c: New test.
> > 	* gcc.dg/live-patching-2.c: New test.
> > 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
> > 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
> > 

> From e44d8b88ac5fb712d5b5e7fdf2f2ad7f43b8ea09 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Fri, 16 Nov 2018 16:23:44 +0100
> Subject: [PATCH] my fixes.
> 
> ---
>  gcc/common.opt      | 3 +--
>  gcc/doc/invoke.texi | 8 ++++----
>  gcc/opts.c          | 2 +-
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 63cd6cc851d..35c24b8e8cf 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2187,8 +2187,7 @@ Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
>  flive-patching=
>  Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_PATCHING_NONE) Optimization
>  -flive-patching=[inline-only-static|inline-clone]	Control IPA
> -optimizations to provide a safe compilation for live-patching. At the same
> -time, provides multiple-level control on the enabled IPA optimizations.
> +optimizations to provide a safe compilation for live-patching.
>  
>  Enum
>  Name(live_patching_level) Type(enum live_patching_level) UnknownError(unknown Live-Patching Level %qs)
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 0fb67163490..9cccc4455fa 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -9288,13 +9288,13 @@ impacted function of the former.  If a function is patched, its impacted
>  functions should be patched too.
>  
>  The impacted functions are decided by the compiler's interprocedural
> -optimizations. For example, inlining a function into its caller, cloning
> +optimizations.  For example, inlining a function into its caller, cloning
>  a function and changing its caller to call this new clone, or extracting
>  a function's pureness/constness information to optimize its direct or
>  indirect callers, etc.
>  
>  Usually, the more IPA optimizations enabled, the larger the number of
> -impacted functions for each function. In order to control the number of
> +impacted functions for each function.  In order to control the number of
>  impacted functions and computed the list of impacted function easily,
>  we provide control to partially enable IPA optimizations on two different
>  levels.
> @@ -9313,8 +9313,8 @@ callers need to be patched as well.
>  @option{-flive-patching=inline-clone} disables the following optimization flags:
>  @gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
>  -fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
> --fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
> --fipa-stack-alignment}
> +-fipa-bit-cp  -fipa-vrp  -fipa-pure-const @gol
> +-fipa-reference-addressable -fipa-stack-alignment}
>  
>  @item inline-only-static
>  
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 570155816e3..0b5e89faeee 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2347,7 +2347,7 @@ common_handle_option (struct gcc_options *opts,
>  
>      case OPT_flive_patching_:
>        if (value)
> -    	control_optimizations_for_live_patching (opts, opts_set,
> +	control_optimizations_for_live_patching (opts, opts_set,
>  						 opts->x_flag_live_patching);
>        break;
>  
> -- 
> 2.19.1
>

Qing Zhao Nov. 16, 2018, 4:04 p.m. UTC | #20

> On Nov 16, 2018, at 9:26 AM, Martin Liška <mliska@suse.cz> wrote:
> 
> On 11/16/18 2:36 AM, Qing Zhao wrote:
>> Hi,
>> 
>> this is the new version of the patch.
>> 
>> I have bootstrapped it on both aarch64 and x86,  no regression.
>> 
>> please take a look.
> 
> Thanks for the updated version of the patch.
> I have last small nits I see:
> 
> - gcc/common.opt: when running --help=common, the line is too long

the following is the output for ./gcc —help=common:
  -flive-patching             Same as -flive-patching=.  Use the latter option
                              instead.
  -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations
                              to provide a safe compilation for live-patching.
                              At the same time, provides multiple-level control
                              on the enabled IPA optimizations.
 
Not sure what’s you mean of “the line is too long”? could you please specify the above which line?

> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
> - gcc/opts.c - do not mix spaces + tabs

I have used contrib/check_GNU_style.sh to check the patch, I did see one place that complains about 2 spaces in between sentences, fixed it.
but I didn’t see spaces + tabs mix issue with the script. could you please specify?

thanks.

Qing

Qing Zhao Nov. 16, 2018, 4:25 p.m. UTC | #21

> On Nov 16, 2018, at 9:51 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
>> On 11/16/18 2:36 AM, Qing Zhao wrote:
>>> Hi,
>>> 
>>> this is the new version of the patch.
>>> 
>>> I have bootstrapped it on both aarch64 and x86,  no regression.
>>> 
>>> please take a look.
>> 
>> Thanks for the updated version of the patch.
>> I have last small nits I see:
>> 
>> - gcc/common.opt: when running --help=common, the line is too long
>> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
>> - gcc/opts.c - do not mix spaces + tabs
>> 
>> With that I'm fine. But note that I'm not a maintainer :)
> 
> I wonder what happens, when I pass like -flive-patching -fwhole-program
> compared to -fwhole-program -flive-patching.
> It seems to me that in first case we will end up with whole-program
> optimization while in the second we won't.
> 
> I guess we should behave in a way that we disable the passes when
> they are enabled implicitly (such as by -O2) but output an error when
> once uses contradicting set of options, lie -flive-patching
> -fwhole-program?

I have thought of this during the implementation, but finally I decided to provide the user 
an opportunity to explicitly enable an ipa optimization if they really want to, even though 
the -flive-patching disables that ipa optimization.   

But I am Okay to change to the behavior you described in the above, I think it’s reasonable too. 

Qing
> 
> Honza
>> 
>> Thanks,
>> Martin
>>

Martin Liška Nov. 19, 2018, 8:11 a.m. UTC | #22

On 11/16/18 5:04 PM, Qing Zhao wrote:
> 
>> On Nov 16, 2018, at 9:26 AM, Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
>>
>> On 11/16/18 2:36 AM, Qing Zhao wrote:
>>> Hi,
>>>
>>> this is the new version of the patch.
>>>
>>> I have bootstrapped it on both aarch64 and x86,  no regression.
>>>
>>> please take a look.
>>
>> Thanks for the updated version of the patch.
>> I have last small nits I see:
>>
>> - gcc/common.opt: when running --help=common, the line is too long
> 
> the following is the output for ./gcc —help=common:
>   -flive-patching             Same as -flive-patching=.  Use the latter option
>                               instead.
>   -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations
>                               to provide a safe compilation for live-patching.
>                               At the same time, provides multiple-level control
>                               on the enabled IPA optimizations.
>  
> Not sure what’s you mean of “the line is too long”? could you please specify the above which line?

You are probably using a console that has quite small column limit, so that you see it automatically
wrapped.

I see:

...
  -flimit-function-alignment  This option lacks documentation.
  -flive-patching             Same as -flive-patching=.  Use the latter option instead.
  -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations to provide a safe compilation for live-patching. At the same time, provides multiple-level control on the enabled IPA optimizations.

^--- the long line

  -flive-range-shrinkage      Relief of register pressure through live range shrinkage.
  -floop-block                Enable loop nest transforms.  Same as -floop-nest-optimize.  Same as -floop-nest-optimize.
...

> 
>> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
>> - gcc/opts.c - do not mix spaces + tabs
> 
> I have used contrib/check_GNU_style.sh to check the patch, I did see one place that complains about 2 spaces in between sentences, fixed it.

I see it:

=== ERROR type #3: dot, space, space, new sentence (3 error(s)) ===
gcc/common.opt:2190:62:optimizations to provide a safe compilation for live-patching.█At the same
gcc/doc/invoke.texi:9291:14:optimizations.█For example, inlining a function into its caller, cloning
gcc/doc/invoke.texi:9297:37:impacted functions for each function.█In order to control the number of

> but I didn’t see spaces + tabs mix issue with the script. could you please specify?

This is a new check that I've just installed:

=== ERROR type #1: a space should not precede a tab (1 error(s)) ===
gcc/opts.c:2350:0:    ████████control_optimizations_for_live_patching (opts, opts_set,

Martin

> 
> thanks.
> 
> Qing
> *
> *
>

Qing Zhao Nov. 19, 2018, 3:52 p.m. UTC | #23

> On Nov 19, 2018, at 2:11 AM, Martin Liška <mliska@suse.cz> wrote:
> 
> On 11/16/18 5:04 PM, Qing Zhao wrote:
>> 
>>> On Nov 16, 2018, at 9:26 AM, Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
>>> 
>>> On 11/16/18 2:36 AM, Qing Zhao wrote:
>>>> Hi,
>>>> 
>>>> this is the new version of the patch.
>>>> 
>>>> I have bootstrapped it on both aarch64 and x86,  no regression.
>>>> 
>>>> please take a look.
>>> 
>>> Thanks for the updated version of the patch.
>>> I have last small nits I see:
>>> 
>>> - gcc/common.opt: when running --help=common, the line is too long
>> 
>> the following is the output for ./gcc —help=common:
>>   -flive-patching             Same as -flive-patching=.  Use the latter option
>>                               instead.
>>   -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations
>>                               to provide a safe compilation for live-patching.
>>                               At the same time, provides multiple-level control
>>                               on the enabled IPA optimizations.
>>  
>> Not sure what’s you mean of “the line is too long”? could you please specify the above which line?
> 
> You are probably using a console that has quite small column limit, so that you see it automatically
> wrapped.
> 
> I see:
> 
> ...
>  -flimit-function-alignment  This option lacks documentation.
>  -flive-patching             Same as -flive-patching=.  Use the latter option instead.
>  -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations to provide a safe compilation for live-patching. At the same time, provides multiple-level control on the enabled IPA optimizations.
> 
> ^--- the long line

Okay, I see.

From the documentation: https://gcc.gnu.org/onlinedocs/gcc-4.3.4/gccint/Option-file-format.html#Option-file-format

"
An option definition record. �These records have the following fields:
	• the name of the option, with the leading “-” removed
	• a space-separated list of option properties (see Option properties)
	• the help text to use for --help (omitted if the second field contains the Undocumented property).
….
The help text is automatically line-wrapped before being displayed. Normally the name of the option is printed on the left-hand side of the output and the help text is printed on the right. However, if the help text contains a tab character, the text to the left of the tab is used instead of the option's name and the text to the right of the tab forms the help text. This allows you to elaborate on what type of argument the option takes.       
“

Looks like that by design, the help text will be automatically line-wrapped before being displayed to fit on the current console. So, I think that the long line should be fine? (the only way to
make the help text shorter line is to cut the help text). 

I also see some other options have even longer help text:

  -fcf-protection=[full|branch|return|none] Instrument functions with checks to verify jump/call/return control-flow transfer instructions have valid targets.
  -fisolate-erroneous-paths-attribute Detect paths that trigger erroneous or undefined behavior due to a null value being used in a way forbidden by a returns_nonnull or nonnull
                              attribute.  Isolate those paths from the main control flow and turn the statement with erroneous or undefined behavior into a trap.
  -fisolate-erroneous-paths-dereference Detect paths that trigger erroneous or undefined behavior due to dereferencing a null pointer.  Isolate those paths from the main control flow
                              and turn the statement with erroneous or undefined behavior into a trap.

> ...
> 
>> 
>>> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
>>> - gcc/opts.c - do not mix spaces + tabs
>> 
>> I have used contrib/check_GNU_style.sh to check the patch, I did see one place that complains about 2 spaces in between sentences, fixed it.
> 
> I see it:
> 
> === ERROR type #3: dot, space, space, new sentence (3 error(s)) ===
> gcc/common.opt:2190:62:optimizations to provide a safe compilation for live-patching.█At the same
> gcc/doc/invoke.texi:9291:14:optimizations.█For example, inlining a function into its caller, cloning
> gcc/doc/invoke.texi:9297:37:impacted functions for each function.█In order to control the number of

fixed.

> 
>> but I didn’t see spaces + tabs mix issue with the script. could you please specify?
> 
> This is a new check that I've just installed:
> 
> === ERROR type #1: a space should not precede a tab (1 error(s)) ===
> gcc/opts.c:2350:0:    ████████control_optimizations_for_live_patching (opts, opts_set,

Okay, I see, fixed. 

thanks.

Qing
> 
> Martin
> 
>> 
>> thanks.
>> 
>> Qing
>> *
>> *

Martin Liška Nov. 20, 2018, 12:10 p.m. UTC | #24

On 11/19/18 4:52 PM, Qing Zhao wrote:
> 
>> On Nov 19, 2018, at 2:11 AM, Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
>>
>> On 11/16/18 5:04 PM, Qing Zhao wrote:
>>>
>>>> On Nov 16, 2018, at 9:26 AM, Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz> <mailto:mliska@suse.cz>> wrote:
>>>>
>>>> On 11/16/18 2:36 AM, Qing Zhao wrote:
>>>>> Hi,
>>>>>
>>>>> this is the new version of the patch.
>>>>>
>>>>> I have bootstrapped it on both aarch64 and x86,  no regression.
>>>>>
>>>>> please take a look.
>>>>
>>>> Thanks for the updated version of the patch.
>>>> I have last small nits I see:
>>>>
>>>> - gcc/common.opt: when running --help=common, the line is too long
>>>
>>> the following is the output for ./gcc —help=common:
>>>   -flive-patching             Same as -flive-patching=.  Use the latter option
>>>                               instead.
>>>   -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations
>>>                               to provide a safe compilation for live-patching.
>>>                               At the same time, provides multiple-level control
>>>                               on the enabled IPA optimizations.
>>>  
>>> Not sure what’s you mean of “the line is too long”? could you please specify the above which line?
>>
>> You are probably using a console that has quite small column limit, so that you see it automatically
>> wrapped.
>>
>> I see:
>>
>> ...
>>  -flimit-function-alignment  This option lacks documentation.
>>  -flive-patching             Same as -flive-patching=.  Use the latter option instead.
>>  -flive-patching=[inline-only-static|inline-clone] Control IPA optimizations to provide a safe compilation for live-patching. At the same time, provides multiple-level control on the enabled IPA optimizations.
>>
>> ^--- the long line
> 
> Okay, I see.
> 
> From the documentation: https://gcc.gnu.org/onlinedocs/gcc-4.3.4/gccint/Option-file-format.html#Option-file-format
> 
> "
> An option definition record. �These records have the following fields:
> • the name of the option, with the leading “-” removed
> • a space-separated list of option properties (see Option properties)
> • the help text to use for --help (omitted if the second field contains the Undocumented property).
> ….
> The help text is automatically line-wrapped before being displayed. Normally the name of the option is printed on the left-hand side of the output and the help text is printed on the right. However, if the help text contains a tab character, the text to the left of the tab is used instead of the option's name and the text to the right of the tab forms the help text. This allows you to elaborate on what type of argument the option takes.       
> “
> 
> Looks like that by design, the help text will be automatically line-wrapped before being displayed to fit on the current console. So, I think that the long line should be fine? (the only way to
> make the help text shorter line is to cut the help text). 

Hi.

I see, then it's all fine.

> 
> I also see some other options have even longer help text:
> 
>   -fcf-protection=[full|branch|return|none] Instrument functions with checks to verify jump/call/return control-flow transfer instructions have valid targets.
>   -fisolate-erroneous-paths-attribute Detect paths that trigger erroneous or undefined behavior due to a null value being used in a way forbidden by a returns_nonnull or nonnull
>                               attribute.  Isolate those paths from the main control flow and turn the statement with erroneous or undefined behavior into a trap.
>   -fisolate-erroneous-paths-dereference Detect paths that trigger erroneous or undefined behavior due to dereferencing a null pointer.  Isolate those paths from the main control flow
>                               and turn the statement with erroneous or undefined behavior into a trap.
> 

Good.

>> ...
>>
>>>
>>>> - gcc/doc/invoke.texi - 2 spaces in between sentences + better gol
>>>> - gcc/opts.c - do not mix spaces + tabs
>>>
>>> I have used contrib/check_GNU_style.sh to check the patch, I did see one place that complains about 2 spaces in between sentences, fixed it.
>>
>> I see it:
>>
>> === ERROR type #3: dot, space, space, new sentence (3 error(s)) ===
>> gcc/common.opt:2190:62:optimizations to provide a safe compilation for live-patching.█At the same
>> gcc/doc/invoke.texi:9291:14:optimizations.█For example, inlining a function into its caller, cloning
>> gcc/doc/invoke.texi:9297:37:impacted functions for each function.█In order to control the number of
> 
> fixed.
> 
>>
>>> but I didn’t see spaces + tabs mix issue with the script. could you please specify?
>>
>> This is a new check that I've just installed:
>>
>> === ERROR type #1: a space should not precede a tab (1 error(s)) ===
>> gcc/opts.c:2350:0:    ████████control_optimizations_for_live_patching (opts, opts_set,
> 
> Okay, I see, fixed. 
> 
> thanks.
> 
> Qing
>>
>> Martin
>>
>>>
>>> thanks.
>>>
>>> Qing
>>> *
>>> *
> 

So please send final version of the patch.

Martin

Qing Zhao Nov. 20, 2018, 3:32 p.m. UTC | #25

Hi,

this is the newest version of the patch. 
major changes:
	1. format fixes raised by Martin;
	2. output error when disabled IPA optimizations are turned on explicitly by the user 
	with -flive-patching at the same time. based on Honza’s suggestions. 

the new changes have been bootstrapped and tested on both aarch64 and x86, no regressions.

Okay for commit?

thanks.

Qing.

===========
gcc/ChangeLog:

2018-11-20  qing zhao  <qing.zhao@oracle.com>

	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
	* common.opt: Add -flive-patching flag.
	* doc/invoke.texi: Document -flive-patching.
	* flag-types.h (enum live_patching_level): New enum.
	* ipa-inline.c (can_inline_edge_p): Disable external functions from
	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
	* opts.c (control_options_for_live_patching): New function.
	(finish_options): Make flag_live_patching incompatible with flag_lto.
	Control IPA optimizations based on different levels of 
	flag_live_patching.

gcc/testsuite/ChangeLog:

2018-11-20  qing zhao  <qing.zhao@oracle.com>

	* gcc.dg/live-patching-1.c: New test.
	* gcc.dg/live-patching-2.c: New test.
	* gcc.dg/live-patching-3.c: New test.
	* gcc.dg/tree-ssa/writeonly-3.c: New test.
	* gcc.target/i386/ipa-stack-alignment-2.c: New test.

Qing Zhao Nov. 26, 2018, 3:54 p.m. UTC | #26

Hi, 
I’d like to ping the following patch for adding -flive-patching master option.

https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01729.html <https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01729.html>

thanks.

Qing




> On Nov 20, 2018, at 9:32 AM, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
> Hi,
> 
> this is the newest version of the patch. 
> major changes:
> 	1. format fixes raised by Martin;
> 	2. output error when disabled IPA optimizations are turned on explicitly by the user 
> 	with -flive-patching at the same time. based on Honza’s suggestions. 
> 
> the new changes have been bootstrapped and tested on both aarch64 and x86, no regressions.
> 
> Okay for commit?
> 
> thanks.
> 
> Qing.
> 
> ===========
> gcc/ChangeLog:
> 
> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
> 
> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
> 	* common.opt: Add -flive-patching flag.
> 	* doc/invoke.texi: Document -flive-patching.
> 	* flag-types.h (enum live_patching_level): New enum.
> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
> 	* opts.c (control_options_for_live_patching): New function.
> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
> 	Control IPA optimizations based on different levels of 
> 	flag_live_patching.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
> 
> 	* gcc.dg/live-patching-1.c: New test.
> 	* gcc.dg/live-patching-2.c: New test.
> 	* gcc.dg/live-patching-3.c: New test.
> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
> 
> <flive-patching-new.patch>

Jan Hubicka Nov. 28, 2018, 3:52 p.m. UTC | #27

> 
> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
> 
> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
> 	* common.opt: Add -flive-patching flag.
> 	* doc/invoke.texi: Document -flive-patching.
> 	* flag-types.h (enum live_patching_level): New enum.
> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
> 	* opts.c (control_options_for_live_patching): New function.
> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
> 	Control IPA optimizations based on different levels of 
> 	flag_live_patching.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
> 
> 	* gcc.dg/live-patching-1.c: New test.
> 	* gcc.dg/live-patching-2.c: New test.
> 	* gcc.dg/live-patching-3.c: New test.
> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
> 

I am still somewhat worried about possible use with C++ programs where
we will kill all inlining of comdats, but I guess we could discuss that
when it becomes an issue.
+
+      /* FIXME: disable unreachable code removal.  */

Disabling unreachable code removal will really introduce a lot of extra
dead code, can't live patches just provide what they need if the code
was earlier removed.
+
+      /* discovery of functions/variables with no address taken.  */
+      if (opts_set->x_flag_ipa_reference_addressable
+	  && opts->x_flag_ipa_reference_addressable)
+	error_at (loc,
+		  "%<-fipa-reference-addressable%> is incompatible with "
+		  "%<-flive-patching=inline-only-static|inline-clone%>");
+      else
+	opts->x_flag_ipa_reference_addressable = 0;
+
+      /* ipa stack alignment propagation.  */
+      if (opts_set->x_flag_ipa_stack_alignment
+	  && opts->x_flag_ipa_stack_alignment)
+	error_at (loc,
+		  "%<-fipa-stack-alignment%> is incompatible with "
+		  "%<-flive-patching=inline-only-static|inline-clone%>");
+      else
+	opts->x_flag_ipa_stack_alignment = 0;

Shall we also disable nothrow or we will worry about C++ only ter?

Patch is OK,
thanks!
Honza

Qing Zhao Nov. 28, 2018, 8:24 p.m. UTC | #28

> On Nov 28, 2018, at 9:52 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
>> 
>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>> 
>> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
>> 	* common.opt: Add -flive-patching flag.
>> 	* doc/invoke.texi: Document -flive-patching.
>> 	* flag-types.h (enum live_patching_level): New enum.
>> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
>> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
>> 	* opts.c (control_options_for_live_patching): New function.
>> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
>> 	Control IPA optimizations based on different levels of 
>> 	flag_live_patching.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>> 
>> 	* gcc.dg/live-patching-1.c: New test.
>> 	* gcc.dg/live-patching-2.c: New test.
>> 	* gcc.dg/live-patching-3.c: New test.
>> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
>> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
>> 
> 
> I am still somewhat worried about possible use with C++ programs where
> we will kill all inlining of comdats, but I guess we could discuss that
> when it becomes an issue.

Okay. If this will be a problem later when we use live-patching in more C++ applications, let’s 
revisit it at that time. 

> +
> +      /* FIXME: disable unreachable code removal.  */
> 
> Disabling unreachable code removal will really introduce a lot of extra
> dead code, can't live patches just provide what they need if the code
> was earlier removed.
> +
> +      /* discovery of functions/variables with no address taken.  */
> +      if (opts_set->x_flag_ipa_reference_addressable
> +	  && opts->x_flag_ipa_reference_addressable)
> +	error_at (loc,
> +		  "%<-fipa-reference-addressable%> is incompatible with "
> +		  "%<-flive-patching=inline-only-static|inline-clone%>");
> +      else
> +	opts->x_flag_ipa_reference_addressable = 0;
> +
> +      /* ipa stack alignment propagation.  */
> +      if (opts_set->x_flag_ipa_stack_alignment
> +	  && opts->x_flag_ipa_stack_alignment)
> +	error_at (loc,
> +		  "%<-fipa-stack-alignment%> is incompatible with "
> +		  "%<-flive-patching=inline-only-static|inline-clone%>");
> +      else
> +	opts->x_flag_ipa_stack_alignment = 0;
> 
> Shall we also disable nothrow or we will worry about C++ only ter?

This is also mainly for C++ applications, so currently should not be a problem.
But I can add a separate simple patch to add another flag to control nothrow propagation and disable it when -flive-patching is ON.

> 
> Patch is OK,

thanks for the review.

I will commit the patch very soon.

Qing
> thanks!
> Honza

Qing Zhao Nov. 29, 2018, 4:15 p.m. UTC | #29

the patch has been committed today as:

https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=266627 <https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=266627>

I will try to update the gcc9 change page soon.

thanks.

Qing

> On Nov 28, 2018, at 2:24 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
>> 
>> On Nov 28, 2018, at 9:52 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> 
>>> 
>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>> 
>>> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
>>> 	* common.opt: Add -flive-patching flag.
>>> 	* doc/invoke.texi: Document -flive-patching.
>>> 	* flag-types.h (enum live_patching_level): New enum.
>>> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
>>> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
>>> 	* opts.c (control_options_for_live_patching): New function.
>>> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
>>> 	Control IPA optimizations based on different levels of 
>>> 	flag_live_patching.
>>> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>> 
>>> 	* gcc.dg/live-patching-1.c: New test.
>>> 	* gcc.dg/live-patching-2.c: New test.
>>> 	* gcc.dg/live-patching-3.c: New test.
>>> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
>>> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
>>> 
>> 
>> I am still somewhat worried about possible use with C++ programs where
>> we will kill all inlining of comdats, but I guess we could discuss that
>> when it becomes an issue.
> 
> Okay. If this will be a problem later when we use live-patching in more C++ applications, let’s 
> revisit it at that time. 
> 
>> +
>> +      /* FIXME: disable unreachable code removal.  */
>> 
>> Disabling unreachable code removal will really introduce a lot of extra
>> dead code, can't live patches just provide what they need if the code
>> was earlier removed.
>> +
>> +      /* discovery of functions/variables with no address taken.  */
>> +      if (opts_set->x_flag_ipa_reference_addressable
>> +	  && opts->x_flag_ipa_reference_addressable)
>> +	error_at (loc,
>> +		  "%<-fipa-reference-addressable%> is incompatible with "
>> +		  "%<-flive-patching=inline-only-static|inline-clone%>");
>> +      else
>> +	opts->x_flag_ipa_reference_addressable = 0;
>> +
>> +      /* ipa stack alignment propagation.  */
>> +      if (opts_set->x_flag_ipa_stack_alignment
>> +	  && opts->x_flag_ipa_stack_alignment)
>> +	error_at (loc,
>> +		  "%<-fipa-stack-alignment%> is incompatible with "
>> +		  "%<-flive-patching=inline-only-static|inline-clone%>");
>> +      else
>> +	opts->x_flag_ipa_stack_alignment = 0;
>> 
>> Shall we also disable nothrow or we will worry about C++ only ter?
> 
> This is also mainly for C++ applications, so currently should not be a problem.
> But I can add a separate simple patch to add another flag to control nothrow propagation and disable it when -flive-patching is ON.
> 
>> 
>> Patch is OK,
> 
> thanks for the review.
> 
> I will commit the patch very soon.
> 
> Qing
>> thanks!
>> Honza

Qing Zhao Dec. 5, 2018, 11:16 p.m. UTC | #30

Hi, Honza,

I have one question relate to whether to disable nothrow for -flive-patching as following:

actually, there are two passes here:

1. local nothrow pass:  in this pass, nothrow attribute is set locally after analyzing every stmt of the function
body: 

unsigned int
pass_nothrow::execute (function *)
{
  struct cgraph_node *node;
  basic_block this_block;
…

}

2. nothrow propagation pass:  (it’s included in the ipa_pure_const pass as following, propagate the nothrow 
attribute along callcgraph)

unsigned int
pass_ipa_pure_const::
execute (function *)
{
  bool remove_p;

  /* Nothrow makes more function to not lead to return and improve
     later analysis.  */
  propagate_nothrow ();
  propagate_malloc ();
  remove_p = propagate_pure_const ();

  delete funct_state_summaries;
  return remove_p ? TODO_remove_functions : 0;
}

the nothrow propagation pass is included in ipa_pure_const pass, and is guarded by flag_ipa_pure_const, 
this flag_ipa_pure_const is disabled when -flive-patching is ON.

So, my question is:

shall we disable local nothrow pass as well when -flive-patching is ON?

my understanding is: we should. 

the reason is:   the local nothrow pass is setting the nothrow attribute for the current routine based on its
body,  and this “nothrow” attribute will be used  when generating EH code around callsite from the caller
of the routine. so the nothrow attribute might impact other routines than the current routine.  as a result,
we should disable this pass?

what’s your opinion on this?

thanks.

Qing

> On Nov 28, 2018, at 2:24 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>> Shall we also disable nothrow or we will worry about C++ only ter?
> 
> This is also mainly for C++ applications, so currently should not be a problem.
> But I can add a separate simple patch to add another flag to control nothrow propagation and disable it when -flive-patching is ON.
> 
>> 
>> Patch is OK,
> 
> thanks for the review.
> 
> I will commit the patch very soon.
> 
> Qing
>> thanks!
>> Honza

Rainer Orth Dec. 7, 2018, 1:07 p.m. UTC | #31

Hi Qing,

>> On Nov 28, 2018, at 9:52 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> 
>>> 
>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>> 
>>> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
>>> 	* common.opt: Add -flive-patching flag.
>>> 	* doc/invoke.texi: Document -flive-patching.
>>> 	* flag-types.h (enum live_patching_level): New enum.
>>> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
>>> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
>>> 	* opts.c (control_options_for_live_patching): New function.
>>> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
>>> 	Control IPA optimizations based on different levels of 
>>> 	flag_live_patching.
>>> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>> 
>>> 	* gcc.dg/live-patching-1.c: New test.
>>> 	* gcc.dg/live-patching-2.c: New test.
>>> 	* gcc.dg/live-patching-3.c: New test.
>>> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
>>> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
[...]
>> Patch is OK,
>
> thanks for the review.
>
> I will commit the patch very soon.

the new gcc.target/i386/ipa-stack-alignment-2.c testcase FAILs on
Solaris/x86:

FAIL: gcc.target/i386/ipa-stack-alignment-2.c scan-assembler sub.*%.sp

Like ipa-stack-alignment.c, it needs -fomit-frame-pointer.  Fixed as
follows, tested on i386-pc-solaris2.11 and x86_64-pc-linux-gnu, 32 and
64-bit each.  Installed on mainline.

	Rainer

Qing Zhao Dec. 7, 2018, 3:14 p.m. UTC | #32

thanks a lot for fixing this issue.

Qing
> On Dec 7, 2018, at 7:07 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
> 
> Hi Qing,
> 
>>> On Nov 28, 2018, at 9:52 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> 
>>>> 
>>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>>> 
>>>> 	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
>>>> 	* common.opt: Add -flive-patching flag.
>>>> 	* doc/invoke.texi: Document -flive-patching.
>>>> 	* flag-types.h (enum live_patching_level): New enum.
>>>> 	* ipa-inline.c (can_inline_edge_p): Disable external functions from
>>>> 	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
>>>> 	* opts.c (control_options_for_live_patching): New function.
>>>> 	(finish_options): Make flag_live_patching incompatible with flag_lto.
>>>> 	Control IPA optimizations based on different levels of 
>>>> 	flag_live_patching.
>>>> 
>>>> gcc/testsuite/ChangeLog:
>>>> 
>>>> 2018-11-20  qing zhao  <qing.zhao@oracle.com>
>>>> 
>>>> 	* gcc.dg/live-patching-1.c: New test.
>>>> 	* gcc.dg/live-patching-2.c: New test.
>>>> 	* gcc.dg/live-patching-3.c: New test.
>>>> 	* gcc.dg/tree-ssa/writeonly-3.c: New test.
>>>> 	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
> [...]
>>> Patch is OK,
>> 
>> thanks for the review.
>> 
>> I will commit the patch very soon.
> 
> the new gcc.target/i386/ipa-stack-alignment-2.c testcase FAILs on
> Solaris/x86:
> 
> FAIL: gcc.target/i386/ipa-stack-alignment-2.c scan-assembler sub.*%.sp
> 
> Like ipa-stack-alignment.c, it needs -fomit-frame-pointer.  Fixed as
> follows, tested on i386-pc-solaris2.11 and x86_64-pc-linux-gnu, 32 and
> 64-bit each.  Installed on mainline.
> 
> 	Rainer
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2018-12-06  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	* gcc.target/i386/ipa-stack-alignment-2.c: Add
> 	-fomit-frame-pointer to dg-options.
> 
> # HG changeset patch
> # Parent  580b8c24b018674f1ffd56a9c672129f746682c5
> Build gcc.target/i386/ipa-stack-alignment-2.c with -fomit-frame-pointer
> 
> diff --git a/gcc/testsuite/gcc.target/i386/ipa-stack-alignment-2.c b/gcc/testsuite/gcc.target/i386/ipa-stack-alignment-2.c
> --- a/gcc/testsuite/gcc.target/i386/ipa-stack-alignment-2.c
> +++ b/gcc/testsuite/gcc.target/i386/ipa-stack-alignment-2.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-flive-patching -O" } */
> +/* { dg-options "-flive-patching -O -fomit-frame-pointer" } */
> 
> typedef struct {
>   long a;

Martin Liška Jan. 2, 2019, 11:47 a.m. UTC | #33

Honza: PING

On 12/6/18 12:16 AM, Qing Zhao wrote:
> Hi, Honza,
> 
> I have one question relate to whether to disable nothrow for -flive-patching as following:
> 
> actually, there are two passes here:
> 
> 1. local nothrow pass:  in this pass, nothrow attribute is set locally after analyzing every stmt of the function
> body: 
> 
> unsigned int
> pass_nothrow::execute (function *)
> {
>   struct cgraph_node *node;
>   basic_block this_block;
> …
> 
> }
> 
> 2. nothrow propagation pass:  (it’s included in the ipa_pure_const pass as following, propagate the nothrow 
> attribute along callcgraph)
> 
> unsigned int
> pass_ipa_pure_const::
> execute (function *)
> {
>   bool remove_p;
> 
>   /* Nothrow makes more function to not lead to return and improve
>      later analysis.  */
>   propagate_nothrow ();
>   propagate_malloc ();
>   remove_p = propagate_pure_const ();
> 
>   delete funct_state_summaries;
>   return remove_p ? TODO_remove_functions : 0;
> }
> 
> the nothrow propagation pass is included in ipa_pure_const pass, and is guarded by flag_ipa_pure_const, 
> this flag_ipa_pure_const is disabled when -flive-patching is ON.
> 
> 
> So, my question is:
> 
> shall we disable local nothrow pass as well when -flive-patching is ON?
> 
> my understanding is: we should. 
> 
> the reason is:   the local nothrow pass is setting the nothrow attribute for the current routine based on its
> body,  and this “nothrow” attribute will be used  when generating EH code around callsite from the caller
> of the routine. so the nothrow attribute might impact other routines than the current routine.  as a result,
> we should disable this pass?
> 
> what’s your opinion on this?
> 
> thanks.
> 
> Qing
> 
>> On Nov 28, 2018, at 2:24 PM, Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>> wrote:
>>>
>>> Shall we also disable nothrow or we will worry about C++ only ter?
>>
>> This is also mainly for C++ applications, so currently should not be a problem.
>> But I can add a separate simple patch to add another flag to control nothrow propagation and disable it when -flive-patching is ON.
>>
>>>
>>> Patch is OK,
>>
>> thanks for the review.
>>
>> I will commit the patch very soon.
>>
>> Qing
>>> thanks!
>>> Honza
>

Martin Liška Jan. 9, 2019, 11:27 a.m. UTC | #34

Honza: PING^2

On 1/2/19 12:47 PM, Martin Liška wrote:
> Honza: PING
> 
> On 12/6/18 12:16 AM, Qing Zhao wrote:
>> Hi, Honza,
>>
>> I have one question relate to whether to disable nothrow for -flive-patching as following:
>>
>> actually, there are two passes here:
>>
>> 1. local nothrow pass:  in this pass, nothrow attribute is set locally after analyzing every stmt of the function
>> body: 
>>
>> unsigned int
>> pass_nothrow::execute (function *)
>> {
>>   struct cgraph_node *node;
>>   basic_block this_block;
>> …
>>
>> }
>>
>> 2. nothrow propagation pass:  (it’s included in the ipa_pure_const pass as following, propagate the nothrow 
>> attribute along callcgraph)
>>
>> unsigned int
>> pass_ipa_pure_const::
>> execute (function *)
>> {
>>   bool remove_p;
>>
>>   /* Nothrow makes more function to not lead to return and improve
>>      later analysis.  */
>>   propagate_nothrow ();
>>   propagate_malloc ();
>>   remove_p = propagate_pure_const ();
>>
>>   delete funct_state_summaries;
>>   return remove_p ? TODO_remove_functions : 0;
>> }
>>
>> the nothrow propagation pass is included in ipa_pure_const pass, and is guarded by flag_ipa_pure_const, 
>> this flag_ipa_pure_const is disabled when -flive-patching is ON.
>>
>>
>> So, my question is:
>>
>> shall we disable local nothrow pass as well when -flive-patching is ON?
>>
>> my understanding is: we should. 
>>
>> the reason is:   the local nothrow pass is setting the nothrow attribute for the current routine based on its
>> body,  and this “nothrow” attribute will be used  when generating EH code around callsite from the caller
>> of the routine. so the nothrow attribute might impact other routines than the current routine.  as a result,
>> we should disable this pass?
>>
>> what’s your opinion on this?
>>
>> thanks.
>>
>> Qing
>>
>>> On Nov 28, 2018, at 2:24 PM, Qing Zhao <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>> wrote:
>>>>
>>>> Shall we also disable nothrow or we will worry about C++ only ter?
>>>
>>> This is also mainly for C++ applications, so currently should not be a problem.
>>> But I can add a separate simple patch to add another flag to control nothrow propagation and disable it when -flive-patching is ON.
>>>
>>>>
>>>> Patch is OK,
>>>
>>> thanks for the review.
>>>
>>> I will commit the patch very soon.
>>>
>>> Qing
>>>> thanks!
>>>> Honza
>>

Jonathan Wakely April 10, 2019, 2:13 p.m. UTC | #35

On 20/11/18 09:32 -0600, Qing Zhao wrote:
>Hi,
>
>this is the newest version of the patch.
>major changes:
>	1. format fixes raised by Martin;
>	2. output error when disabled IPA optimizations are turned on explicitly by the user
>	with -flive-patching at the same time. based on Honza’s suggestions.
>
>the new changes have been bootstrapped and tested on both aarch64 and x86, no regressions.
>
>Okay for commit?
>
>thanks.
>
>Qing.
>
>===========
>gcc/ChangeLog:
>
>2018-11-20  qing zhao  <qing.zhao@oracle.com>
>
>	* cif-code.def (EXTERN_LIVE_ONLY_STATIC): New CIF code.
>	* common.opt: Add -flive-patching flag.
>	* doc/invoke.texi: Document -flive-patching.
>	* flag-types.h (enum live_patching_level): New enum.
>	* ipa-inline.c (can_inline_edge_p): Disable external functions from
>	inlining when flag_live_patching is LIVE_PATCHING_INLINE_ONLY_STATIC.
>	* opts.c (control_options_for_live_patching): New function.
>	(finish_options): Make flag_live_patching incompatible with flag_lto.
>	Control IPA optimizations based on different levels of
>	flag_live_patching.
>
>gcc/testsuite/ChangeLog:
>
>2018-11-20  qing zhao  <qing.zhao@oracle.com>
>
>	* gcc.dg/live-patching-1.c: New test.
>	* gcc.dg/live-patching-2.c: New test.
>	* gcc.dg/live-patching-3.c: New test.
>	* gcc.dg/tree-ssa/writeonly-3.c: New test.
>	* gcc.target/i386/ipa-stack-alignment-2.c: New test.
>


Hi Qing Zhao,

I was looking at the new docs for -flive-patching and have some
questions.

>--- a/gcc/doc/invoke.texi
>+++ b/gcc/doc/invoke.texi
>@@ -416,6 +416,7 @@ Objective-C and Objective-C++ Dialects}.
> -fipa-bit-cp -fipa-vrp @gol
> -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
> -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
>+-flive-patching=@var{level} @gol
> -fira-region=@var{region}  -fira-hoist-pressure @gol
> -fira-loop-pressure  -fno-ira-share-save-slots @gol
> -fno-ira-share-spill-slots @gol
>@@ -9045,6 +9046,65 @@ equivalences that are found only by GCC and equivalences found only by Gold.
> 
> This flag is enabled by default at @option{-O2} and @option{-Os}.
> 
>+@item -flive-patching=@var{level}
>+@opindex flive-patching
>+Control GCC's optimizations to provide a safe compilation for live-patching.

"provide a safe compilation" isn't very clear to me. I don't know what
it means to "provide a compilation", let alone a safe one.

Could we say something like "Control GCC’s optimizations to produce
output suitable for live-patching." ?


>+If the compiler's optimization uses a function's body or information extracted
>+from its body to optimize/change another function, the latter is called an
>+impacted function of the former.  If a function is patched, its impacted
>+functions should be patched too.
>+
>+The impacted functions are decided by the compiler's interprocedural

decided or determined?

>+optimizations.  For example, inlining a function into its caller, cloning
>+a function and changing its caller to call this new clone, or extracting
>+a function's pureness/constness information to optimize its direct or
>+indirect callers, etc.

I don't know what the second sentence is saying. I can read it two
different ways:

1) Those are examples of interprocedural optimizations which
participate in the decision making, but the actual details of how the
decisions are made are not specified here.

2) Performing those optimizations causes a function to be impacted.

If 1) is the intended meaning, then I think it should say "For
example, <INS>when</INS> inlining a function into its caller, ..."

If 2) is the intended meaning, then I think it should say "For
example, <INS>a caller is impacted when</INS> inlining a function
into its caller ...".

Does either of those suggestions match the intended meaning? Or do you
have a better way to rephrase it?

>+Usually, the more IPA optimizations enabled, the larger the number of
>+impacted functions for each function.  In order to control the number of
>+impacted functions and computed the list of impacted function easily,

Should be "and more easily compute the list of impacted functions".

>+we provide control to partially enable IPA optimizations on two different
>+levels.

We don't usually say "we provide" like this. I suggest simply "IPA
optimizations can be partially enabled at two different levels."

>+
>+The @var{level} argument should be one of the following:
>+
>+@table @samp
>+
>+@item inline-clone
>+
>+Only enable inlining and cloning optimizations, which includes inlining,
>+cloning, interprocedural scalar replacement of aggregates and partial inlining.
>+As a result, when patching a function, all its callers and its clones'
>+callers need to be patched as well.

Since you've defined the term "impacted" could this just say "all its
callers and its clones' callers are impacted."?

>+@option{-flive-patching=inline-clone} disables the following optimization flags:
>+@gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
>+-fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
>+-fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
>+-fipa-stack-alignment}
>+
>+@item inline-only-static
>+
>+Only enable inlining of static functions.
>+As a result, when patching a static function, all its callers need to be
>+patches as well.

"Typo: "patches" should be "patched", but I'd suggest "are impacted"
here too.

>+In addition to all the flags that -flive-patching=inline-clone disables,
>+@option{-flive-patching=inline-only-static} disables the following additional
>+optimization flags:
>+@gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
>+
>+@end table
>+
>+When -flive-patching specified without any value, the default value
>+is "inline-clone".

This should use @option{} and @var{} and is missing the word "is".

>+This flag is disabled by default.
>+
>+Note that -flive-patching is not supported with link-time optimizer.

s/optimizer./optimization/

>+(@option{-flto}).
>+
> @item -fisolate-erroneous-paths-dereference
> @opindex fisolate-erroneous-paths-dereference
> Detect paths that trigger erroneous or undefined behavior due to

The attached patch makes some of these changes, but I'd like to know
if my changes preserve the intended meaning.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 73a7789d879..1e9f0fa7230 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9367,7 +9367,7 @@ This flag is enabled by default at @option{-O2} and @option{-Os}.
 
 @item -flive-patching=@var{level}
 @opindex flive-patching
-Control GCC's optimizations to provide a safe compilation for live-patching.
+Control GCC's optimizations to produce output suitable for live-patching.
 
 If the compiler's optimization uses a function's body or information extracted
 from its body to optimize/change another function, the latter is called an
@@ -9375,16 +9375,16 @@ impacted function of the former.  If a function is patched, its impacted
 functions should be patched too.
 
 The impacted functions are decided by the compiler's interprocedural
-optimizations.  For example, inlining a function into its caller, cloning
-a function and changing its caller to call this new clone, or extracting
-a function's pureness/constness information to optimize its direct or
-indirect callers, etc.
+optimizations.  For example, a caller is impacted when inlining a function
+into its caller,
+cloning a function and changing its caller to call this new clone,
+or extracting a function's pureness/constness information to optimize
+its direct or indirect callers, etc.
 
 Usually, the more IPA optimizations enabled, the larger the number of
 impacted functions for each function.  In order to control the number of
-impacted functions and computed the list of impacted function easily,
-we provide control to partially enable IPA optimizations on two different
-levels.
+impacted functions and more easily compute the list of impacted function,
+IPA optimizations can be partially enabled at two different levels.
 
 The @var{level} argument should be one of the following:
 
@@ -9407,21 +9407,22 @@ callers need to be patched as well.
 
 Only enable inlining of static functions.
 As a result, when patching a static function, all its callers need to be
-patches as well.
+patched as well.
 
-In addition to all the flags that -flive-patching=inline-clone disables,
+In addition to all the flags that @option{-flive-patching=inline-clone}
+disables,
 @option{-flive-patching=inline-only-static} disables the following additional
 optimization flags:
 @gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
 
 @end table
 
-When -flive-patching specified without any value, the default value
-is "inline-clone".
+When @option{-flive-patching} is specified without any value, the default value
+is @var{inline-clone}.
 
 This flag is disabled by default.
 
-Note that -flive-patching is not supported with link-time optimizer.
+Note that @option{-flive-patching} is not supported with link-time optimization
 (@option{-flto}).
 
 @item -fisolate-erroneous-paths-dereference

Qing Zhao April 10, 2019, 6:55 p.m. UTC | #36

Hi, Jonathan,

thanks for your review on the documentation change for -flive-patching option.

> 
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -416,6 +416,7 @@ Objective-C and Objective-C++ Dialects}.
>> -fipa-bit-cp -fipa-vrp @gol
>> -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
>> -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
>> +-flive-patching=@var{level} @gol
>> -fira-region=@var{region}  -fira-hoist-pressure @gol
>> -fira-loop-pressure  -fno-ira-share-save-slots @gol
>> -fno-ira-share-spill-slots @gol
>> @@ -9045,6 +9046,65 @@ equivalences that are found only by GCC and equivalences found only by Gold.
>> This flag is enabled by default at @option{-O2} and @option{-Os}.
>> +@item -flive-patching=@var{level}
>> +@opindex flive-patching
>> +Control GCC's optimizations to provide a safe compilation for live-patching.
> 
> "provide a safe compilation" isn't very clear to me. I don't know what
> it means to "provide a compilation", let alone a safe one.
> 
> Could we say something like "Control GCC’s optimizations to produce
> output suitable for live-patching.” ?

yes, this is better.

> 
> 
>> +If the compiler's optimization uses a function's body or information extracted
>> +from its body to optimize/change another function, the latter is called an
>> +impacted function of the former.  If a function is patched, its impacted
>> +functions should be patched too.
>> +
>> +The impacted functions are decided by the compiler's interprocedural
> 
> decided or determined?
determined is better.

> 
>> +optimizations.  For example, inlining a function into its caller, cloning
>> +a function and changing its caller to call this new clone, or extracting
>> +a function's pureness/constness information to optimize its direct or
>> +indirect callers, etc.
> 
> I don't know what the second sentence is saying. I can read it two
> different ways:
> 
> 1) Those are examples of interprocedural optimizations which
> participate in the decision making, but the actual details of how the
> decisions are made are not specified here.
> 
> 2) Performing those optimizations causes a function to be impacted.
> 
> If 1) is the intended meaning, then I think it should say "For
> example, <INS>when</INS> inlining a function into its caller, ..."
> 
> If 2) is the intended meaning, then I think it should say "For
> example, <INS>a caller is impacted when</INS> inlining a function
> into its caller …".

2) is the intended meaining.

> 
> Does either of those suggestions match the intended meaning? Or do you
> have a better way to rephrase it?
> 
>> +Usually, the more IPA optimizations enabled, the larger the number of
>> +impacted functions for each function.  In order to control the number of
>> +impacted functions and computed the list of impacted function easily,
> 
> Should be "and more easily compute the list of impacted functions”.

this is good.
> 
>> +we provide control to partially enable IPA optimizations on two different
>> +levels.
> 
> We don't usually say "we provide" like this. I suggest simply "IPA
> optimizations can be partially enabled at two different levels.”

Okay.
> 
>> +
>> +The @var{level} argument should be one of the following:
>> +
>> +@table @samp
>> +
>> +@item inline-clone
>> +
>> +Only enable inlining and cloning optimizations, which includes inlining,
>> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
>> +As a result, when patching a function, all its callers and its clones'
>> +callers need to be patched as well.
> 
> Since you've defined the term "impacted" could this just say "all its
> callers and its clones' callers are impacted.”?

I think that the following might be better:

when patching a function, all its callers and its clones’ callers are impacted, therefore need to be patched as well.

> 
>> +@option{-flive-patching=inline-clone} disables the following optimization flags:
>> +@gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
>> +-fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
>> +-fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
>> +-fipa-stack-alignment}
>> +
>> +@item inline-only-static
>> +
>> +Only enable inlining of static functions.
>> +As a result, when patching a static function, all its callers need to be
>> +patches as well.
> 
> "Typo: "patches" should be "patched", but I'd suggest "are impacted"
> here too.

Okay.
> 
>> +In addition to all the flags that -flive-patching=inline-clone disables,
>> +@option{-flive-patching=inline-only-static} disables the following additional
>> +optimization flags:
>> +@gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
>> +
>> +@end table
>> +
>> +When -flive-patching specified without any value, the default value
>> +is "inline-clone".
> 
> This should use @option{} and @var{} and is missing the word "is”.
Okay.
> 
>> +This flag is disabled by default.
>> +
>> +Note that -flive-patching is not supported with link-time optimizer.
> 
> s/optimizer./optimization/
Okay.
> 
>> +(@option{-flto}).
>> +
>> @item -fisolate-erroneous-paths-dereference
>> @opindex fisolate-erroneous-paths-dereference
>> Detect paths that trigger erroneous or undefined behavior due to
> 
> The attached patch makes some of these changes, but I'd like to know
> if my changes preserve the intended meaning.

the changes in the patch looks good to me.

thanks a lot.

Qing
> 
> <patch.txt>

Jonathan Wakely April 10, 2019, 7:49 p.m. UTC | #37

On 10/04/19 13:55 -0500, Qing Zhao wrote:
>Hi, Jonathan,
>
>thanks for your review on the documentation change for -flive-patching option.
>
>>
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -416,6 +416,7 @@ Objective-C and Objective-C++ Dialects}.
>>> -fipa-bit-cp -fipa-vrp @gol
>>> -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
>>> -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
>>> +-flive-patching=@var{level} @gol
>>> -fira-region=@var{region}  -fira-hoist-pressure @gol
>>> -fira-loop-pressure  -fno-ira-share-save-slots @gol
>>> -fno-ira-share-spill-slots @gol
>>> @@ -9045,6 +9046,65 @@ equivalences that are found only by GCC and equivalences found only by Gold.
>>> This flag is enabled by default at @option{-O2} and @option{-Os}.
>>> +@item -flive-patching=@var{level}
>>> +@opindex flive-patching
>>> +Control GCC's optimizations to provide a safe compilation for live-patching.
>>
>> "provide a safe compilation" isn't very clear to me. I don't know what
>> it means to "provide a compilation", let alone a safe one.
>>
>> Could we say something like "Control GCC’s optimizations to produce
>> output suitable for live-patching.” ?
>
>yes, this is better.
>
>>
>>
>>> +If the compiler's optimization uses a function's body or information extracted
>>> +from its body to optimize/change another function, the latter is called an
>>> +impacted function of the former.  If a function is patched, its impacted
>>> +functions should be patched too.
>>> +
>>> +The impacted functions are decided by the compiler's interprocedural
>>
>> decided or determined?
>determined is better.
>
>>
>>> +optimizations.  For example, inlining a function into its caller, cloning
>>> +a function and changing its caller to call this new clone, or extracting
>>> +a function's pureness/constness information to optimize its direct or
>>> +indirect callers, etc.
>>
>> I don't know what the second sentence is saying. I can read it two
>> different ways:
>>
>> 1) Those are examples of interprocedural optimizations which
>> participate in the decision making, but the actual details of how the
>> decisions are made are not specified here.
>>
>> 2) Performing those optimizations causes a function to be impacted.
>>
>> If 1) is the intended meaning, then I think it should say "For
>> example, <INS>when</INS> inlining a function into its caller, ..."
>>
>> If 2) is the intended meaning, then I think it should say "For
>> example, <INS>a caller is impacted when</INS> inlining a function
>> into its caller …".
>
>2) is the intended meaining.
>
>>
>> Does either of those suggestions match the intended meaning? Or do you
>> have a better way to rephrase it?
>>
>>> +Usually, the more IPA optimizations enabled, the larger the number of
>>> +impacted functions for each function.  In order to control the number of
>>> +impacted functions and computed the list of impacted function easily,
>>
>> Should be "and more easily compute the list of impacted functions”.
>
>this is good.
>>
>>> +we provide control to partially enable IPA optimizations on two different
>>> +levels.
>>
>> We don't usually say "we provide" like this. I suggest simply "IPA
>> optimizations can be partially enabled at two different levels.”
>
>Okay.
>>
>>> +
>>> +The @var{level} argument should be one of the following:
>>> +
>>> +@table @samp
>>> +
>>> +@item inline-clone
>>> +
>>> +Only enable inlining and cloning optimizations, which includes inlining,
>>> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
>>> +As a result, when patching a function, all its callers and its clones'
>>> +callers need to be patched as well.
>>
>> Since you've defined the term "impacted" could this just say "all its
>> callers and its clones' callers are impacted.”?
>
>I think that the following might be better:
>
>when patching a function, all its callers and its clones’ callers are impacted, therefore need to be patched as well.

Agreed.

>>
>>> +@option{-flive-patching=inline-clone} disables the following optimization flags:
>>> +@gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
>>> +-fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
>>> +-fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
>>> +-fipa-stack-alignment}
>>> +
>>> +@item inline-only-static
>>> +
>>> +Only enable inlining of static functions.
>>> +As a result, when patching a static function, all its callers need to be
>>> +patches as well.
>>
>> "Typo: "patches" should be "patched", but I'd suggest "are impacted"
>> here too.
>
>Okay.
>>
>>> +In addition to all the flags that -flive-patching=inline-clone disables,
>>> +@option{-flive-patching=inline-only-static} disables the following additional
>>> +optimization flags:
>>> +@gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
>>> +
>>> +@end table
>>> +
>>> +When -flive-patching specified without any value, the default value
>>> +is "inline-clone".
>>
>> This should use @option{} and @var{} and is missing the word "is”.
>Okay.
>>
>>> +This flag is disabled by default.
>>> +
>>> +Note that -flive-patching is not supported with link-time optimizer.
>>
>> s/optimizer./optimization/
>Okay.
>>
>>> +(@option{-flto}).
>>> +
>>> @item -fisolate-erroneous-paths-dereference
>>> @opindex fisolate-erroneous-paths-dereference
>>> Detect paths that trigger erroneous or undefined behavior due to
>>
>> The attached patch makes some of these changes, but I'd like to know
>> if my changes preserve the intended meaning.
>
>the changes in the patch looks good to me.
>
>thanks a lot.

Thanks!

I've attached an updated patch with your suggestions.

Reviewers, is this OK for trunk?
commit 0b9a201fb80fb1e708d83566df50f1555cf80e10
Author: nickc <nickc@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Apr 10 14:44:47 2019 +0000

    Clarify documentation for -flive-patching
    
            * doc/invoke.texi (Optimize Options): Clarify -flive-patching docs.

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 755b9f754a1..3a88d8db157 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9367,24 +9367,24 @@ This flag is enabled by default at @option{-O2} and @option{-Os}.
 
 @item -flive-patching=@var{level}
 @opindex flive-patching
-Control GCC's optimizations to provide a safe compilation for live-patching.
+Control GCC's optimizations to produce output suitable for live-patching.
 
 If the compiler's optimization uses a function's body or information extracted
 from its body to optimize/change another function, the latter is called an
 impacted function of the former.  If a function is patched, its impacted
 functions should be patched too.
 
-The impacted functions are decided by the compiler's interprocedural
-optimizations.  For example, inlining a function into its caller, cloning
-a function and changing its caller to call this new clone, or extracting
-a function's pureness/constness information to optimize its direct or
-indirect callers, etc.
+The impacted functions are determined by the compiler's interprocedural
+optimizations.  For example, a caller is impacted when inlining a function
+into its caller,
+cloning a function and changing its caller to call this new clone,
+or extracting a function's pureness/constness information to optimize
+its direct or indirect callers, etc.
 
 Usually, the more IPA optimizations enabled, the larger the number of
 impacted functions for each function.  In order to control the number of
-impacted functions and computed the list of impacted function easily,
-we provide control to partially enable IPA optimizations on two different
-levels.
+impacted functions and more easily compute the list of impacted function,
+IPA optimizations can be partially enabled at two different levels.
 
 The @var{level} argument should be one of the following:
 
@@ -9395,7 +9395,7 @@ The @var{level} argument should be one of the following:
 Only enable inlining and cloning optimizations, which includes inlining,
 cloning, interprocedural scalar replacement of aggregates and partial inlining.
 As a result, when patching a function, all its callers and its clones'
-callers need to be patched as well.
+callers are impacted, therefore need to be patched as well.
 
 @option{-flive-patching=inline-clone} disables the following optimization flags:
 @gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
@@ -9406,22 +9406,23 @@ callers need to be patched as well.
 @item inline-only-static
 
 Only enable inlining of static functions.
-As a result, when patching a static function, all its callers need to be
-patches as well.
+As a result, when patching a static function, all its callers are impacted
+and so need to be patched as well.
 
-In addition to all the flags that -flive-patching=inline-clone disables,
+In addition to all the flags that @option{-flive-patching=inline-clone}
+disables,
 @option{-flive-patching=inline-only-static} disables the following additional
 optimization flags:
 @gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
 
 @end table
 
-When -flive-patching specified without any value, the default value
-is "inline-clone".
+When @option{-flive-patching} is specified without any value, the default value
+is @var{inline-clone}.
 
 This flag is disabled by default.
 
-Note that -flive-patching is not supported with link-time optimizer.
+Note that @option{-flive-patching} is not supported with link-time optimization
 (@option{-flto}).
 
 @item -fisolate-erroneous-paths-dereference

Richard Biener April 11, 2019, 7:58 a.m. UTC | #38

On Wed, Apr 10, 2019 at 9:49 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 10/04/19 13:55 -0500, Qing Zhao wrote:
> >Hi, Jonathan,
> >
> >thanks for your review on the documentation change for -flive-patching option.
> >
> >>
> >>> --- a/gcc/doc/invoke.texi
> >>> +++ b/gcc/doc/invoke.texi
> >>> @@ -416,6 +416,7 @@ Objective-C and Objective-C++ Dialects}.
> >>> -fipa-bit-cp -fipa-vrp @gol
> >>> -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
> >>> -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
> >>> +-flive-patching=@var{level} @gol
> >>> -fira-region=@var{region}  -fira-hoist-pressure @gol
> >>> -fira-loop-pressure  -fno-ira-share-save-slots @gol
> >>> -fno-ira-share-spill-slots @gol
> >>> @@ -9045,6 +9046,65 @@ equivalences that are found only by GCC and equivalences found only by Gold.
> >>> This flag is enabled by default at @option{-O2} and @option{-Os}.
> >>> +@item -flive-patching=@var{level}
> >>> +@opindex flive-patching
> >>> +Control GCC's optimizations to provide a safe compilation for live-patching.
> >>
> >> "provide a safe compilation" isn't very clear to me. I don't know what
> >> it means to "provide a compilation", let alone a safe one.
> >>
> >> Could we say something like "Control GCC’s optimizations to produce
> >> output suitable for live-patching.” ?
> >
> >yes, this is better.
> >
> >>
> >>
> >>> +If the compiler's optimization uses a function's body or information extracted
> >>> +from its body to optimize/change another function, the latter is called an
> >>> +impacted function of the former.  If a function is patched, its impacted
> >>> +functions should be patched too.
> >>> +
> >>> +The impacted functions are decided by the compiler's interprocedural
> >>
> >> decided or determined?
> >determined is better.
> >
> >>
> >>> +optimizations.  For example, inlining a function into its caller, cloning
> >>> +a function and changing its caller to call this new clone, or extracting
> >>> +a function's pureness/constness information to optimize its direct or
> >>> +indirect callers, etc.
> >>
> >> I don't know what the second sentence is saying. I can read it two
> >> different ways:
> >>
> >> 1) Those are examples of interprocedural optimizations which
> >> participate in the decision making, but the actual details of how the
> >> decisions are made are not specified here.
> >>
> >> 2) Performing those optimizations causes a function to be impacted.
> >>
> >> If 1) is the intended meaning, then I think it should say "For
> >> example, <INS>when</INS> inlining a function into its caller, ..."
> >>
> >> If 2) is the intended meaning, then I think it should say "For
> >> example, <INS>a caller is impacted when</INS> inlining a function
> >> into its caller …".
> >
> >2) is the intended meaining.
> >
> >>
> >> Does either of those suggestions match the intended meaning? Or do you
> >> have a better way to rephrase it?
> >>
> >>> +Usually, the more IPA optimizations enabled, the larger the number of
> >>> +impacted functions for each function.  In order to control the number of
> >>> +impacted functions and computed the list of impacted function easily,
> >>
> >> Should be "and more easily compute the list of impacted functions”.
> >
> >this is good.
> >>
> >>> +we provide control to partially enable IPA optimizations on two different
> >>> +levels.
> >>
> >> We don't usually say "we provide" like this. I suggest simply "IPA
> >> optimizations can be partially enabled at two different levels.”
> >
> >Okay.
> >>
> >>> +
> >>> +The @var{level} argument should be one of the following:
> >>> +
> >>> +@table @samp
> >>> +
> >>> +@item inline-clone
> >>> +
> >>> +Only enable inlining and cloning optimizations, which includes inlining,
> >>> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
> >>> +As a result, when patching a function, all its callers and its clones'
> >>> +callers need to be patched as well.
> >>
> >> Since you've defined the term "impacted" could this just say "all its
> >> callers and its clones' callers are impacted.”?
> >
> >I think that the following might be better:
> >
> >when patching a function, all its callers and its clones’ callers are impacted, therefore need to be patched as well.
>
> Agreed.
>
> >>
> >>> +@option{-flive-patching=inline-clone} disables the following optimization flags:
> >>> +@gccoptlist{-fwhole-program  -fipa-pta  -fipa-reference  -fipa-ra @gol
> >>> +-fipa-icf  -fipa-icf-functions  -fipa-icf-variables @gol
> >>> +-fipa-bit-cp  -fipa-vrp  -fipa-pure-const  -fipa-reference-addressable @gol
> >>> +-fipa-stack-alignment}
> >>> +
> >>> +@item inline-only-static
> >>> +
> >>> +Only enable inlining of static functions.
> >>> +As a result, when patching a static function, all its callers need to be
> >>> +patches as well.
> >>
> >> "Typo: "patches" should be "patched", but I'd suggest "are impacted"
> >> here too.
> >
> >Okay.
> >>
> >>> +In addition to all the flags that -flive-patching=inline-clone disables,
> >>> +@option{-flive-patching=inline-only-static} disables the following additional
> >>> +optimization flags:
> >>> +@gccoptlist{-fipa-cp-clone  -fipa-sra  -fpartial-inlining  -fipa-cp}
> >>> +
> >>> +@end table
> >>> +
> >>> +When -flive-patching specified without any value, the default value
> >>> +is "inline-clone".
> >>
> >> This should use @option{} and @var{} and is missing the word "is”.
> >Okay.
> >>
> >>> +This flag is disabled by default.
> >>> +
> >>> +Note that -flive-patching is not supported with link-time optimizer.
> >>
> >> s/optimizer./optimization/
> >Okay.
> >>
> >>> +(@option{-flto}).
> >>> +
> >>> @item -fisolate-erroneous-paths-dereference
> >>> @opindex fisolate-erroneous-paths-dereference
> >>> Detect paths that trigger erroneous or undefined behavior due to
> >>
> >> The attached patch makes some of these changes, but I'd like to know
> >> if my changes preserve the intended meaning.
> >
> >the changes in the patch looks good to me.
> >
> >thanks a lot.
>
> Thanks!
>
> I've attached an updated patch with your suggestions.
>
> Reviewers, is this OK for trunk?

OK (it's an improvement at least).

Richard.

>

[RFC] Come up with -flive-patching master option.

Commit Message

Comments

Patch