On Wed, Apr 10, 2019 at 9:49 PM Jonathan Wakely <jwakely@xxxxxxxxxx> 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.
>
