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>
