From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116783 invoked by alias); 15 Nov 2018 08:41:44 -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 116759 invoked by uid 89); 15 Nov 2018 08:41:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=UD:ipa-stack-alignment.c, sk:ipa-sta, sk:ipasta X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Nov 2018 08:41:41 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 52A43AE78; Thu, 15 Nov 2018 08:41:39 +0000 (UTC) Subject: Re: [PATCH][RFC] Come up with -flive-patching master option. To: Qing Zhao Cc: Miroslav Benes , Jan Hubicka , Martin Jambor , live-patching@vger.kernel.org, gcc Patches References: <1a023bdc-28a6-eb41-b449-4d096f12064f@suse.cz> <3E37D3A8-2D19-41C2-BA8A-8F0EFA1B4D5C@oracle.com> <10a54034-279b-a406-8466-55558effbf24@suse.cz> <20181003090457.GJ57692@kam.mff.cuni.cz> <54a75932-201b-671c-0a63-d1a5d8d7b562@suse.cz> <90c91045-cb9d-0bd2-fad3-d16426ceede6@suse.cz> <20181105095135.j3mnzox6rkktkoto@kam.mff.cuni.cz> <629b20d2-7b0b-9342-a64a-d12eb6e6a6b7@suse.cz> <20181108145909.5eh5ccen7a7elspe@kam.mff.cuni.cz> <173116c6-d51c-3bef-5382-38593e032f82@suse.cz> <8f467934-c088-9b60-c4d3-c19c2e03defd@suse.cz> <2F74DD05-E807-4FB4-80B1-3BBC6FEDF6E8@oracle.com> <7FA7E4B3-BF39-4E60-84AD-91161CC4867F@oracle.com> <8F7AEFF2-5DCE-49B6-BC9E-34FC0DEF2A55@oracle.com> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <568496ae-24cf-7453-57db-a2188d5d11c2@suse.cz> Date: Thu, 15 Nov 2018 08:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <8F7AEFF2-5DCE-49B6-BC9E-34FC0DEF2A55@oracle.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg01350.txt.bz2 On 11/14/18 6:54 PM, Qing Zhao wrote: > Hi, > > >> On Nov 14, 2018, at 9:03 AM, Martin Liška wrote: >> >>>> >>> Yes, you are right. I added this into my patch. >>> >>> I am attaching the new patch here. >> >> Hello. >> >> Please use >> git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py /tmp/patch >> >> in order to address many formatting issues of the patch (skip the ones reported in common.opt). > > will do and fix the style issues. > >> >>> >>> >>> +flive-patching >>> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization >>> + >>> +flive-patching= >>> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE) Optimization >>> +-flive-patching=[inline-only-static|inline-clone] Control ipa optimizations to provide a >> >> Please use 'IPA' instead of 'ipa', similarly in documentation. > Okay. >> >>> --- a/gcc/doc/invoke.texi >>> +++ b/gcc/doc/invoke.texi >>> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}. >>> -fgcse-sm -fhoist-adjacent-loads -fif-conversion @gol >>> -fif-conversion2 -findirect-inlining @gol >>> -finline-functions -finline-functions-called-once -finline-limit=@var{n} @gol >>> --finline-small-functions -fipa-cp -fipa-cp-clone @gol >>> +-finline-small-functions -fipa-cp -fipa-cp-clone @gol >> >> This changes is probably not intended. > No. will delete it. > >> >>> >>> diff --git a/gcc/flag-types.h b/gcc/flag-types.h >>> index 500f663..72e0f0f 100644 >>> --- a/gcc/flag-types.h >>> +++ b/gcc/flag-types.h >>> @@ -123,6 +123,14 @@ enum stack_reuse_level >>> SR_ALL >>> }; >>> >>> +/* The live patching level. */ >>> +enum live_patching_level >>> +{ >>> + LIVE_NONE = 0, >>> + LIVE_INLINE_ONLY_STATIC, >>> + LIVE_INLINE_CLONE >> >> Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum >> values are bit unclear. > > Okay. >> >>> >>> + /* visibility change should be excluded by !flag_whole_program >>> + && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra >> >> You added sorry about LTO, maybe then !in_lto_p would be always true? > > Yes, since live-patching does not support LTO currently, !in_lto_p is always TRUE. that’s the reason no need for a new flag to disable visibility change. Hi. Ok. >> >>> + && !flag_partial_inlining. */ >>> + if (!opts_set->x_flag_ipa_pta) >>> + opts->x_flag_ipa_pta = 0; >>> + if (!opts_set->x_flag_ipa_reference) >>> + opts->x_flag_ipa_reference = 0; >>> + if (!opts_set->x_flag_ipa_ra) >>> + opts->x_flag_ipa_ra = 0; >>> + if (!opts_set->x_flag_ipa_icf) >>> + opts->x_flag_ipa_icf = 0; >>> + if (!opts_set->x_flag_ipa_icf_functions) >>> + opts->x_flag_ipa_icf_functions = 0; >>> + if (!opts_set->x_flag_ipa_icf_variables) >>> + opts->x_flag_ipa_icf_variables = 0; >>> + if (!opts_set->x_flag_ipa_bit_cp) >>> + opts->x_flag_ipa_bit_cp = 0; >>> + if (!opts_set->x_flag_ipa_vrp) >>> + opts->x_flag_ipa_vrp = 0; >>> + if (!opts_set->x_flag_ipa_pure_const) >> >> Can you please explain why you included: >> if (!opts_set->x_flag_ipa_bit_cp) >> opts->x_flag_ipa_bit_cp = 0; >> if (!opts_set->x_flag_ipa_vrp) >> opts->x_flag_ipa_vrp = 0; > > interprocedural bitwise constant propagation and interprocedural propagation of value ranges does not involve creating clones, > and the bitwise constant and value ranges info extracted during ipa-cp phase are used later by other optimizations. their effect on > impact functions are not clear at this moment. that’s the reason I think we need to disable these two. > > Martin Jambor raised this issue during our previous discussion on 10/03/2018: > “ > I was thinking a bit more about this and recalled that not all stuff > that IPA-CP nowadays does involves creating clones, so we have to add > also: > - -fno-ipa-bit-cp, and > - -fno-ipa-vrp. > > These two just record info in the parameters of *callees* of functions > from which it extracted info, without any cloning involved. Both were > introduced in GCC 7. > > Thanks, > > Martin > “ > and I think he is right. Great, thanks for clarification! I forgot about that. And please can you mention in documentation which options are disabled with -flive-patching=*? We usually do it, e.g. take a look at '-Os' option: ``` ‘-Os’ disables the following optimization flags: -falign-functions -falign-jumps -falign-loops -falign-labels -freorder-blocks -freorder-blocks-algorithm=stc -freorder-blocks-and-partition -fprefetch-loop-arrays ``` > > > >> ? >> >>> + opts->x_flag_ipa_pure_const = 0; >>> + /* unreachable code removal. */ >>> + /* discovery of functions/variables with no address taken. */ >> >> ^^^ these 2 comments looks misaligned. > > will fix them. >> >>> >>> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c >>> @@ -0,0 +1,22 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline" } */ >>> + >>> +extern int sum, n, m; >>> + >>> +int foo (int a) >>> +{ >>> + return a + n; >>> +} >>> + >>> +static int bar (int b) >>> +{ >>> + return b * m; >>> +} >>> + >>> +int main() >>> +{ >>> + sum = foo (m) + bar (n); >>> + return 0; >>> +} >>> + >>> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when the user requests only inlining static for live patching" "inline" } } */ >>> -- 1.9.1 >> >> It would be also handy to test the newly added option. > > not sure how to test it? any suggestion? I would just e.g. copy test for recently added gcc.target/i386/ipa-stack-alignment.c and replace the option with corresponding -flive-patching option. > >> Please add ChangeLog entry for the patch. > > Okay. That will help you to set up a skeleton: ./contrib/mklog /tmp/patch > /tmp/changelog > >> Have you bootstrapped the patch and run test-suite? > did on aarch64. I will do it on x86_64 as well. Good, please mentioned that when sending a patch next time. One more nit, please use gcc_unreachable instead of gcc_assert (0). Thanks for working on that, Martin > > thanks. > > Qing >