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

Message ID e68f7f65-5e71-3f2b-4f47-fcbb90395af4@suse.cz
State New
Headers show
Series
  • [RFC] Come up with -flive-patching master option.
Related show

Commit Message

Martin Liška Nov. 9, 2018, 3:33 p.m.
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

Comments

Qing Zhao Nov. 9, 2018, 5:43 p.m. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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
>>

Patch

From dd52cd0249fc30cf6d7bf01a8826323277817b78 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 7 Nov 2018 12:41:19 +0100
Subject: [PATCH] Come up with -fvectorized-functions.

---
 gcc/fortran/decl.c            | 33 +++++++++++++++++++++++++++
 gcc/fortran/gfortran.h        |  2 ++
 gcc/fortran/match.h           |  1 +
 gcc/fortran/parse.c           |  3 +++
 gcc/fortran/trans-intrinsic.c | 43 +++++++++++++++++++++++++++++++++++
 5 files changed, 82 insertions(+)

diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index 2b77d950abb..938c35c508f 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -98,6 +98,9 @@  bool gfc_matching_function;
 /* Set upon parsing a !GCC$ unroll n directive for use in the next loop.  */
 int directive_unroll = -1;
 
+/* List middle-end built-ins that should be vectorized.  */
+vec<const char *> vectorized_builtins;
+
 /* If a kind expression of a component of a parameterized derived type is
    parameterized, temporarily store the expression here.  */
 static gfc_expr *saved_kind_expr = NULL;
@@ -11243,3 +11246,33 @@  gfc_match_gcc_unroll (void)
   gfc_error ("Syntax error in !GCC$ UNROLL directive at %C");
   return MATCH_ERROR;
 }
+
+/* Match a !GCC$ builtin b attributes flags form:
+
+   The parameter b is name of a middle-end built-in.
+   Flags are one of:
+     - omp-simd-notinbranch.
+
+   When we come here, we have already matched the !GCC$ builtin string.  */
+match
+gfc_match_gcc_builtin (void)
+{
+  char builtin[GFC_MAX_SYMBOL_LEN + 1];
+
+  if (gfc_match_name (builtin) != MATCH_YES)
+    return MATCH_ERROR;
+
+  gfc_gobble_whitespace ();
+  if (gfc_match ("attributes") != MATCH_YES)
+    return MATCH_ERROR;
+
+  gfc_gobble_whitespace ();
+  if (gfc_match ("omp_simd_notinbranch") != MATCH_YES)
+    return MATCH_ERROR;
+
+  char *r = XNEWVEC (char, strlen (builtin) + 32);
+  sprintf (r, "__builtin_%s", builtin);
+  vectorized_builtins.safe_push (r);
+
+  return MATCH_YES;
+}
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index d8ef35d9d6c..bb7f4dd0c4b 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -2763,6 +2763,7 @@  gfc_finalizer;
 bool gfc_in_match_data (void);
 match gfc_match_char_spec (gfc_typespec *);
 extern int directive_unroll;
+extern vec<const char *> vectorized_builtins;
 
 /* Handling Parameterized Derived Types  */
 bool gfc_insert_kind_parameter_exprs (gfc_expr *);
@@ -3501,5 +3502,6 @@  bool gfc_is_reallocatable_lhs (gfc_expr *);
 /* trans-decl.c */
 
 void finish_oacc_declare (gfc_namespace *, gfc_symbol *, bool);
+void gfc_adjust_builtins (void);
 
 #endif /* GCC_GFORTRAN_H  */
diff --git a/gcc/fortran/match.h b/gcc/fortran/match.h
index 418542bd5a6..f25ed860c06 100644
--- a/gcc/fortran/match.h
+++ b/gcc/fortran/match.h
@@ -247,6 +247,7 @@  match gfc_match_dimension (void);
 match gfc_match_external (void);
 match gfc_match_gcc_attributes (void);
 match gfc_match_gcc_unroll (void);
+match gfc_match_gcc_builtin (void);
 match gfc_match_import (void);
 match gfc_match_intent (void);
 match gfc_match_intrinsic (void);
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 13cc6f5fccd..56d0d050bc3 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -1072,6 +1072,7 @@  decode_gcc_attribute (void)
 
   match ("attributes", gfc_match_gcc_attributes, ST_ATTR_DECL);
   match ("unroll", gfc_match_gcc_unroll, ST_NONE);
+  match ("builtin", gfc_match_gcc_builtin, ST_NONE);
 
   /* All else has failed, so give up.  See if any of the matchers has
      stored an error message of some sort.  */
@@ -5663,6 +5664,8 @@  parse_progunit (gfc_statement st)
   gfc_state_data *p;
   int n;
 
+  gfc_adjust_builtins ();
+
   if (gfc_new_block
       && gfc_new_block->abr_modproc_decl
       && gfc_new_block->attr.function)
diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 4ae2b3252b5..0417e039a39 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -597,7 +597,50 @@  define_quad_builtin (const char *name, tree type, bool is_const)
   return fndecl;
 }
 
+/* Add SIMD attribute for FNDECL built-in if the built-in
+   name is in VECTORIZED_BUILTINS.  */
 
+static void
+add_simd_flag_for_built_in (tree fndecl)
+{
+  if (fndecl == NULL_TREE)
+    return;
+
+  const char *name = IDENTIFIER_POINTER (DECL_NAME (fndecl));
+  for (unsigned i = 0; i < vectorized_builtins.length (); i++)
+    if (strcmp (vectorized_builtins[i], name) == 0)
+      {
+	DECL_ATTRIBUTES (fndecl)
+	  = tree_cons (get_identifier ("omp declare simd"),
+		       build_tree_list (NULL_TREE,
+					build_omp_clause (UNKNOWN_LOCATION,
+							  OMP_CLAUSE_NOTINBRANCH)),
+		       DECL_ATTRIBUTES (fndecl));
+	return;
+      }
+}
+
+void
+gfc_adjust_builtins (void)
+{
+  gfc_intrinsic_map_t *m;
+  for (m = gfc_intrinsic_map;
+       m->id != GFC_ISYM_NONE || m->double_built_in != END_BUILTINS; m++)
+    {
+      add_simd_flag_for_built_in (m->real4_decl);
+      add_simd_flag_for_built_in (m->complex4_decl);
+      add_simd_flag_for_built_in (m->real8_decl);
+      add_simd_flag_for_built_in (m->complex8_decl);
+      add_simd_flag_for_built_in (m->real10_decl);
+      add_simd_flag_for_built_in (m->complex10_decl);
+      add_simd_flag_for_built_in (m->real16_decl);
+      add_simd_flag_for_built_in (m->complex16_decl);
+      add_simd_flag_for_built_in (m->real16_decl);
+      add_simd_flag_for_built_in (m->complex16_decl);
+    }
+
+  vectorized_builtins.truncate (0);
+}
 
 /* Initialize function decls for library functions.  The external functions
    are created as required.  Builtin functions are added here.  */
-- 
2.19.1