From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64985 invoked by alias); 10 Apr 2019 19:49:32 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 64943 invoked by uid 89); 10 Apr 2019 19:49:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 Apr 2019 19:49:25 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 03CE7307D98B; Wed, 10 Apr 2019 19:49:24 +0000 (UTC) Received: from localhost (unknown [10.33.36.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id A477C19C65; Wed, 10 Apr 2019 19:49:23 +0000 (UTC) Date: Wed, 10 Apr 2019 20:01:00 -0000 From: Jonathan Wakely To: Qing Zhao Cc: Jan Hubicka , Martin =?utf-8?B?TGnFoWth?= , Miroslav Benes , Martin Jambor , live-patching@vger.kernel.org, gcc Patches Subject: Re: [PATCH][Version 3]Come up with -flive-patching master option. Message-ID: <20190410194922.GJ943@redhat.com> References: <49B97110-C7FD-4569-AE26-1B37951D802B@oracle.com> <30e99713-0a96-3acb-ea5f-8f0996be69a6@suse.cz> <5A3304F6-2084-44BA-86CB-88A657AAF741@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> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="XbHSybK3LHOYQtWI" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <54BAB962-66E3-4BC6-BC76-7B1BE5568D85@oracle.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.11.3 (2019-02-01) X-SW-Source: 2019-04/txt/msg00394.txt.bz2 --XbHSybK3LHOYQtWI Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit Content-length: 5784 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? --XbHSybK3LHOYQtWI Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" Content-length: 3854 commit 0b9a201fb80fb1e708d83566df50f1555cf80e10 Author: nickc Date: Wed Apr 10 14:44:47 2019 +0000 Clarify documentation for -flive-patching * doc/invoke.texi (Optimize Options): Clarify -flive-patching docs. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 755b9f754a1..3a88d8db157 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -9367,24 +9367,24 @@ 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. +Control GCC's optimizations to produce output suitable for live-patching. 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 -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. +The impacted functions are determined by the compiler's interprocedural +optimizations. For example, a caller is impacted when 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. 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, -we provide control to partially enable IPA optimizations on two different -levels. +impacted functions and more easily compute the list of impacted function, +IPA optimizations can be partially enabled at two different levels. The @var{level} argument should be one of the following: @@ -9395,7 +9395,7 @@ The @var{level} argument should be one of the following: 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. +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 @@ -9406,22 +9406,23 @@ callers need to be patched as well. @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. +As a result, when patching a static function, all its callers are impacted +and so need to be patched as well. -In addition to all the flags that -flive-patching=inline-clone disables, +In addition to all the flags that @option{-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". +When @option{-flive-patching} is specified without any value, the default value +is @var{inline-clone}. This flag is disabled by default. -Note that -flive-patching is not supported with link-time optimizer. +Note that @option{-flive-patching} is not supported with link-time optimization (@option{-flto}). @item -fisolate-erroneous-paths-dereference --XbHSybK3LHOYQtWI--