From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com [IPv6:2a00:1450:4864:20::62a]) by sourceware.org (Postfix) with ESMTPS id 117D63858405 for ; Tue, 24 Aug 2021 12:13:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 117D63858405 Received: by mail-ej1-x62a.google.com with SMTP id n27so11957013eja.5 for ; Tue, 24 Aug 2021 05:13:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=vJ9uwdBHYZRBEks+PhsSZrvSdvS1LT8eJaN8T2nG1IQ=; b=RxSKj8A67eg+aJ0ucaVKC8llfQLmohaXmE/PZt9AnXYsu2zb3WYRQVDYNgrzsI2X2t UbQnEmdofE0SIwgW4JyGstLp4QcsE4egXH4y0HtijwGxW7MCGfTx/9Qck8rxnUIjtGP1 H5IlPAB5RAFUlDbzyrj1d9byi30eEemzES4pMpFOOOcEWi4jXyYSPPgyCIWTGZgQythK S+QMadKwXsrYjddMGOa/pbrmNjo2z6HKaOC2p5s72HXgVc66TNuuwJcEY2CKv6HumZYv EvrvTtEs04QE1FRcPXowlHbVpom1+9jl3aCl4O/GJlERJfXtDorUJEtbtu5C0KRq8z2G iQJQ== X-Gm-Message-State: AOAM530XrPQGUCFbia8FY9/JYEOj4xZLSHzV9I4PDoyrmG70LJC0MTEw lSf1hENpGVXaFm/3Y3jsPB9L4XzBHHKO9CQ6o5c= X-Google-Smtp-Source: ABdhPJykeiKTCjPOoF6zbvHjyqoc+WmDcyA08ThTInZsqGM9k0IAMOfU3lEcCMaG3EbfunLrfTmUr6+ayJsmEeP/Vpc= X-Received: by 2002:a17:906:3bc3:: with SMTP id v3mr40622597ejf.482.1629807212925; Tue, 24 Aug 2021 05:13:32 -0700 (PDT) MIME-Version: 1.0 References: <82e71ebf-7b2e-67e7-1f08-ea525deee4cb@suse.cz> <1dfa7226-3056-d215-4626-01126d428891@suse.cz> In-Reply-To: <1dfa7226-3056-d215-4626-01126d428891@suse.cz> From: Richard Biener Date: Tue, 24 Aug 2021 14:13:22 +0200 Message-ID: Subject: Re: [PATCH] Optimize macro: make it more predictable To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: GCC Patches , Jakub Jelinek , Michael Matz Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Aug 2021 12:13:44 -0000 On Thu, Jul 1, 2021 at 3:13 PM Martin Li=C5=A1ka wrote: > > On 10/23/20 1:47 PM, Martin Li=C5=A1ka wrote: > > Hey. > > Hello. > > I deferred the patch for GCC 12. Since the time, I messed up with options > I feel more familiar with the option handling. So ... > > > > > This is a follow-up of the discussion that happened in thread about no_= stack_protector > > attribute: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545916.ht= ml > > > > The current optimize attribute works in the following way: > > - 1) we take current global_options as base > > - 2) maybe_default_options is called for the currently selected optimiz= ation level, which > > means all rules in default_options_table are executed > > - 3) attribute values are applied (via decode_options) > > > > So the step 2) is problematic: in case of -O2 -fno-omit-frame-pointer a= nd __attribute__((optimize("-fno-stack-protector"))) > > ends basically with -O2 -fno-stack-protector because -fno-omit-frame-po= inter is default: > > /* -O1 and -Og optimizations. */ > > { OPT_LEVELS_1_PLUS, OPT_fomit_frame_pointer, NULL, 1 }, > > > > My patch handled and the current optimize attribute really behaves that= same as appending attribute value > > to the command line. So far so good. We should also reflect that in doc= umentation entry which is quite > > vague right now: > > ^^^ all these are still valid arguments, plus I'm adding a new test-case = that tests that. There is also handle_common_deferred_options that's not called so any option processed there should probably be excempt from being set/unset in the optimize attribute? > > > > """ > > The optimize attribute is used to specify that a function is to be comp= iled with different optimization options than specified on the command line= . > > """ > > I addressed that with documentation changes, should be more clear to user= s. Moreover, I noticed that we declare 'optimize' attribute > as something not for a production use: > > "The optimize attribute should be used for debugging purposes only. It is= not suitable in production code." > > Are we sure about the statement? I know that e.g. glibc uses that. Well, given we're changing behavior now that warning looks valid ;) I'll also note that "The optimize attribute arguments of a function behave as if they were added to the command line options." is still likely untrue, the global state init is complicated ;) > > > > and we may want to handle -Ox in the attribute in a special way. I gues= s many macro/pragma users expect that > > > > -O2 -ftree-vectorize and __attribute__((optimize(1))) will end with -O1= and not > > with -ftree-vectorize -O1 ? As implemented your patch seems to turn it into -ftree-vectorize -O1. IIRC multiple optimize attributes apply ontop of each other, and it makes sense to me that optimize (2), optimize ("tree-vectorize") behaves the same as optimize (2, "tree-vectorize"). I'm not sure this is still the case after your patch? Also consider #pragma GCC optimize ("tree-vectorize") void foo () { ...} #pragma GCC optimize ("tree-loop-distribution") void bar () {... } I'd expect bar to have both vectorization and loop distribution enabled? (note I didn't use push/pop here) > The situation with 'target' attribute is different. When parsing the attr= ibute, we intentionally drop all existing target flags: > > $ cat -n gcc/config/i386/i386-options.c > ... > 1245 if (opt =3D=3D IX86_FUNCTION_SPECIFIC_ARCH) > 1246 { > 1247 /* If arch=3D is set, clear all bits in x_ix8= 6_isa_flags, > 1248 except for ISA_64BIT, ABI_64, ABI_X32, and = CODE16 > 1249 and all bits in x_ix86_isa_flags2. */ > 1250 opts->x_ix86_isa_flags &=3D (OPTION_MASK_ISA_6= 4BIT > 1251 | OPTION_MASK_ABI_6= 4 > 1252 | OPTION_MASK_ABI_X= 32 > 1253 | OPTION_MASK_CODE1= 6); > 1254 opts->x_ix86_isa_flags_explicit &=3D (OPTION_M= ASK_ISA_64BIT > 1255 | OPTION_M= ASK_ABI_64 > 1256 | OPTION_M= ASK_ABI_X32 > 1257 | OPTION_M= ASK_CODE16); > 1258 opts->x_ix86_isa_flags2 =3D 0; > 1259 opts->x_ix86_isa_flags2_explicit =3D 0; > 1260 } > > That seems logical because target attribute is used for e.g. ifunc multi-= versioning and one needs > to be sure all existing ISA flags are dropped. However, I noticed clang b= ehaves differently: > > $ cat hreset.c > #pragma GCC target "arch=3Dgeode" > #include > void foo(unsigned int eax) > { > _hreset (eax); > } > > $ clang hreset.c -mhreset -c -O2 -m32 > $ gcc hreset.c -mhreset -c -O2 -m32 > In file included from /home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu/= 12.0.0/include/x86gprintrin.h:97, > from /home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu= /12.0.0/include/immintrin.h:27, > from hreset.c:2: > hreset.c: In function =E2=80=98foo=E2=80=99: > /home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu/12.0.0/include/hreseti= ntrin.h:39:1: error: inlining failed in call to =E2=80=98always_inline=E2= =80=99 =E2=80=98_hreset=E2=80=99: target specific option mismatch > 39 | _hreset (unsigned int __EAX) > | ^~~~~~~ > hreset.c:5:3: note: called from here > 5 | _hreset (eax); > | ^~~~~~~~~~~~~ > > Anyway, I think the current target attribute handling should be preserved= . I think this and the -O1 argument above suggests that there should be a way to distinguish two modes - add to the active set of options and starting from scratch. Maybe it's over-designing things but do we want to preserve the existing behavior and instead add optimize ("+ftree-vectorize") and target ("+avx2") as a way to amend the state? OTOH as we're missing global_options re-init even with your patch we won't = get the defaults correct (aka what toplev::main does with init_options_struct a= nd the corresponding langhook). Likewise if lang_hooks.init_options performs = any defaulting a later flag overrides and we override that with optimize() that doesn't work - I'm thinking of things like flag_complex_method and -fcx-* f= lags. So -O2 -fcx-fortran-rules on the command-line and optimize ("no-cx-fortran-rules") to cancel the -fcx-fortran-rules switch wouldn't work? Thanks, Richard. > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > > > > I'm also planning to take a look at the target macro/attribute, I expec= t similar problems: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D97469 > > > > Thoughts? > > Thanks, > > Martin >