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?