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, when inlining a function into its caller, ..."
>>
>> If 2) is the intended meaning, then I think it should say "For
>> example, a caller is impacted when 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?