This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][Version 3]Come up with -flive-patching master option.
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jonathan Wakely <jwakely at redhat dot com>
- Cc: Qing Zhao <qing dot zhao at oracle dot com>, Jan Hubicka <hubicka at ucw dot cz>, Martin Liška <mliska at suse dot cz>, Miroslav Benes <mbenes at suse dot cz>, Martin Jambor <mjambor at suse dot cz>, live-patching at vger dot kernel dot org, gcc Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 11 Apr 2019 09:58:11 +0200
- Subject: Re: [PATCH][Version 3]Come up with -flive-patching master option.
- References: <49B97110-C7FD-4569-AE26-1B37951D802B@oracle.com> <ED839E0B-A332-491E-ADC9-D6EC06BECD27@oracle.com> <30e99713-0a96-3acb-ea5f-8f0996be69a6@suse.cz> <5A3304F6-2084-44BA-86CB-88A657AAF741@oracle.com> <d5af0ffa-88ed-065f-fc6c-811ffd954dd0@suse.cz> <F78B52B9-9F9A-4FA0-91BF-A33307D87AA8@oracle.com> <25de1f37-40b1-1834-78cc-13f89215906d@suse.cz> <06EEEEBB-40EC-47DE-BB30-22784E3E28AF@oracle.com> <20190410141353.GA18869@redhat.com> <54BAB962-66E3-4BC6-BC76-7B1BE5568D85@oracle.com> <20190410194922.GJ943@redhat.com>
On Wed, Apr 10, 2019 at 9:49 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 10/04/19 13:55 -0500, Qing Zhao wrote:
> >Hi, Jonathan,
> >
> >thanks for your review on the documentation change for -flive-patching option.
> >
> >>
> >>> --- a/gcc/doc/invoke.texi
> >>> +++ b/gcc/doc/invoke.texi
> >>> @@ -416,6 +416,7 @@ Objective-C and Objective-C++ Dialects}.
> >>> -fipa-bit-cp -fipa-vrp @gol
> >>> -fipa-pta -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable @gol
> >>> -fipa-stack-alignment -fipa-icf -fira-algorithm=@var{algorithm} @gol
> >>> +-flive-patching=@var{level} @gol
> >>> -fira-region=@var{region} -fira-hoist-pressure @gol
> >>> -fira-loop-pressure -fno-ira-share-save-slots @gol
> >>> -fno-ira-share-spill-slots @gol
> >>> @@ -9045,6 +9046,65 @@ equivalences that are found only by GCC and equivalences found only by Gold.
> >>> This flag is enabled by default at @option{-O2} and @option{-Os}.
> >>> +@item -flive-patching=@var{level}
> >>> +@opindex flive-patching
> >>> +Control GCC's optimizations to provide a safe compilation for live-patching.
> >>
> >> "provide a safe compilation" isn't very clear to me. I don't know what
> >> it means to "provide a compilation", let alone a safe one.
> >>
> >> Could we say something like "Control GCC’s optimizations to produce
> >> output suitable for live-patching.” ?
> >
> >yes, this is better.
> >
> >>
> >>
> >>> +If the compiler's optimization uses a function's body or information extracted
> >>> +from its body to optimize/change another function, the latter is called an
> >>> +impacted function of the former. If a function is patched, its impacted
> >>> +functions should be patched too.
> >>> +
> >>> +The impacted functions are decided by the compiler's interprocedural
> >>
> >> decided or determined?
> >determined is better.
> >
> >>
> >>> +optimizations. For example, inlining a function into its caller, cloning
> >>> +a function and changing its caller to call this new clone, or extracting
> >>> +a function's pureness/constness information to optimize its direct or
> >>> +indirect callers, etc.
> >>
> >> I don't know what the second sentence is saying. I can read it two
> >> different ways:
> >>
> >> 1) Those are examples of interprocedural optimizations which
> >> participate in the decision making, but the actual details of how the
> >> decisions are made are not specified here.
> >>
> >> 2) Performing those optimizations causes a function to be impacted.
> >>
> >> If 1) is the intended meaning, then I think it should say "For
> >> example, <INS>when</INS> inlining a function into its caller, ..."
> >>
> >> If 2) is the intended meaning, then I think it should say "For
> >> example, <INS>a caller is impacted when</INS> inlining a function
> >> into its caller …".
> >
> >2) is the intended meaining.
> >
> >>
> >> Does either of those suggestions match the intended meaning? Or do you
> >> have a better way to rephrase it?
> >>
> >>> +Usually, the more IPA optimizations enabled, the larger the number of
> >>> +impacted functions for each function. In order to control the number of
> >>> +impacted functions and computed the list of impacted function easily,
> >>
> >> Should be "and more easily compute the list of impacted functions”.
> >
> >this is good.
> >>
> >>> +we provide control to partially enable IPA optimizations on two different
> >>> +levels.
> >>
> >> We don't usually say "we provide" like this. I suggest simply "IPA
> >> optimizations can be partially enabled at two different levels.”
> >
> >Okay.
> >>
> >>> +
> >>> +The @var{level} argument should be one of the following:
> >>> +
> >>> +@table @samp
> >>> +
> >>> +@item inline-clone
> >>> +
> >>> +Only enable inlining and cloning optimizations, which includes inlining,
> >>> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
> >>> +As a result, when patching a function, all its callers and its clones'
> >>> +callers need to be patched as well.
> >>
> >> Since you've defined the term "impacted" could this just say "all its
> >> callers and its clones' callers are impacted.”?
> >
> >I think that the following might be better:
> >
> >when patching a function, all its callers and its clones’ callers are impacted, therefore need to be patched as well.
>
> Agreed.
>
> >>
> >>> +@option{-flive-patching=inline-clone} disables the following optimization flags:
> >>> +@gccoptlist{-fwhole-program -fipa-pta -fipa-reference -fipa-ra @gol
> >>> +-fipa-icf -fipa-icf-functions -fipa-icf-variables @gol
> >>> +-fipa-bit-cp -fipa-vrp -fipa-pure-const -fipa-reference-addressable @gol
> >>> +-fipa-stack-alignment}
> >>> +
> >>> +@item inline-only-static
> >>> +
> >>> +Only enable inlining of static functions.
> >>> +As a result, when patching a static function, all its callers need to be
> >>> +patches as well.
> >>
> >> "Typo: "patches" should be "patched", but I'd suggest "are impacted"
> >> here too.
> >
> >Okay.
> >>
> >>> +In addition to all the flags that -flive-patching=inline-clone disables,
> >>> +@option{-flive-patching=inline-only-static} disables the following additional
> >>> +optimization flags:
> >>> +@gccoptlist{-fipa-cp-clone -fipa-sra -fpartial-inlining -fipa-cp}
> >>> +
> >>> +@end table
> >>> +
> >>> +When -flive-patching specified without any value, the default value
> >>> +is "inline-clone".
> >>
> >> This should use @option{} and @var{} and is missing the word "is”.
> >Okay.
> >>
> >>> +This flag is disabled by default.
> >>> +
> >>> +Note that -flive-patching is not supported with link-time optimizer.
> >>
> >> s/optimizer./optimization/
> >Okay.
> >>
> >>> +(@option{-flto}).
> >>> +
> >>> @item -fisolate-erroneous-paths-dereference
> >>> @opindex fisolate-erroneous-paths-dereference
> >>> Detect paths that trigger erroneous or undefined behavior due to
> >>
> >> The attached patch makes some of these changes, but I'd like to know
> >> if my changes preserve the intended meaning.
> >
> >the changes in the patch looks good to me.
> >
> >thanks a lot.
>
> Thanks!
>
> I've attached an updated patch with your suggestions.
>
> Reviewers, is this OK for trunk?
OK (it's an improvement at least).
Richard.
>