From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 9B253385780E for ; Mon, 9 Nov 2020 10:27:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9B253385780E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mliska@suse.cz X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 62AB0AD8C; Mon, 9 Nov 2020 10:27:42 +0000 (UTC) Subject: Re: [PATCH] Optimize macro: make it more predictable To: Richard Biener Cc: GCC Patches , Jakub Jelinek , Michael Matz References: <82e71ebf-7b2e-67e7-1f08-ea525deee4cb@suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <861ab944-4447-7c23-1236-4b6aef202ad0@suse.cz> Date: Mon, 9 Nov 2020 11:27:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 09 Nov 2020 10:27:45 -0000 On 11/3/20 2:27 PM, Richard Biener wrote: > On Fri, Oct 23, 2020 at 1:47 PM Martin Liška wrote: >> >> Hey. >> >> 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.html >> >> 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 optimization 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 and __attribute__((optimize("-fno-stack-protector"))) >> ends basically with -O2 -fno-stack-protector because -fno-omit-frame-pointer 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 documentation entry which is quite >> vague right now: >> >> """ >> The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line. >> """ >> >> and we may want to handle -Ox in the attribute in a special way. I guess many macro/pragma users expect that >> >> -O2 -ftree-vectorize and __attribute__((optimize(1))) will end with -O1 and not >> with -ftree-vectorize -O1 ? > > Hmm. I guess the only two reasonable options are to append to the active set > and thus end up with -ftree-vectorize -O1 or to start from an empty set and thus > end up with -O1. > > Maybe we can have > > @item optimize (@var{level}, @dots{}) > > reset everything to plain -On and > > @item optimize (@var{string}, @dots{}) > > append? So optimize("O1") will end up with -O2 -ftree-vectorize -O1 and > optimize(1) with -O1? How do we handle Hello. To be honest I don't like it much, it can be quite confusing for uses of the GCC. > > void __attribute__((optimize(1),optimize("ftree-vectorize"))) > > thus two optimize attributes? Yes, parse_optimize_options is called twice for the situation. I'm going to reply to Jakub's email ... > >> I'm also planning to take a look at the target macro/attribute, I expect similar problems: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97469 >> >> Thoughts? >> Thanks, >> Martin >> >> gcc/c-family/ChangeLog: >> >> * c-common.c (parse_optimize_options): Decoded attribute options >> with the ones that were already set on the command line. >> >> gcc/ChangeLog: >> >> * toplev.c (toplev::main): Save decoded Optimization options. >> * toplev.h (save_opt_decoded_options): New. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/i386/avx512er-vrsqrt28ps-3.c: Disable -ffast-math. >> * gcc.target/i386/avx512er-vrsqrt28ps-5.c: Likewise. >> --- >> gcc/c-family/c-common.c | 15 ++++++++++++++- >> .../gcc.target/i386/avx512er-vrsqrt28ps-3.c | 2 +- >> .../gcc.target/i386/avx512er-vrsqrt28ps-5.c | 2 +- >> gcc/toplev.c | 8 ++++++++ >> gcc/toplev.h | 1 + >> 5 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c >> index e16ca3894bc..d4342e93d0a 100644 >> --- a/gcc/c-family/c-common.c >> +++ b/gcc/c-family/c-common.c >> @@ -5727,10 +5727,23 @@ parse_optimize_options (tree args, bool attr_p) >> j++; >> } >> decoded_options_count = j; >> + >> + /* Merge the decoded options with save_decoded_options. */ >> + unsigned save_opt_count = save_opt_decoded_options.length (); >> + unsigned merged_decoded_options_count = save_opt_count + decoded_options_count; >> + cl_decoded_option *merged_decoded_options >> + = XNEWVEC (cl_decoded_option, merged_decoded_options_count); >> + >> + for (unsigned i = 0; i < save_opt_count; ++i) >> + merged_decoded_options[i] = save_opt_decoded_options[i]; >> + for (unsigned i = 0; i < decoded_options_count; ++i) >> + merged_decoded_options[save_opt_count + i] = decoded_options[i]; >> + >> /* And apply them. */ >> decode_options (&global_options, &global_options_set, >> - decoded_options, decoded_options_count, >> + merged_decoded_options, merged_decoded_options_count, >> input_location, global_dc, NULL); >> + free (decoded_options); >> >> targetm.override_options_after_change(); >> >> diff --git a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c >> index 1ba8172d6e3..40aefb50844 100644 >> --- a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c >> +++ b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c >> @@ -8,7 +8,7 @@ >> #define MAX 1000 >> #define EPS 0.00001 >> >> -__attribute__ ((noinline, optimize (1))) >> +__attribute__ ((noinline, optimize (1, "-fno-fast-math"))) >> void static >> compute_rsqrt_ref (float *a, float *r) >> { >> diff --git a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c >> index e067a81e562..498f4d50aa5 100644 >> --- a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c >> +++ b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c >> @@ -8,7 +8,7 @@ >> #define MAX 1000 >> #define EPS 0.00001 >> >> -__attribute__ ((noinline, optimize (1))) >> +__attribute__ ((noinline, optimize (1, "-fno-fast-math"))) >> void static >> compute_sqrt_ref (float *a, float *r) >> { >> diff --git a/gcc/toplev.c b/gcc/toplev.c >> index 8c1e1e1f44f..27e4a1bc485 100644 >> --- a/gcc/toplev.c >> +++ b/gcc/toplev.c >> @@ -123,6 +123,9 @@ static bool no_backend; >> struct cl_decoded_option *save_decoded_options; >> unsigned int save_decoded_options_count; >> >> +/* Vector of saved Optimization decoded command line options. */ >> +auto_vec save_opt_decoded_options; >> + >> /* Used to enable -fvar-tracking, -fweb and -frename-registers according >> to optimize in process_options (). */ >> #define AUTODETECT_VALUE 2 >> @@ -2430,6 +2433,11 @@ toplev::main (int argc, char **argv) >> &save_decoded_options, >> &save_decoded_options_count); >> >> + /* Save Optimization decoded options. */ >> + for (unsigned i = 0; i < save_decoded_options_count; ++i) >> + if (cl_options[save_decoded_options[i].opt_index].flags & CL_OPTIMIZATION) >> + save_opt_decoded_options.safe_push (save_decoded_options[i]); >> + >> /* Perform language-specific options initialization. */ >> lang_hooks.init_options (save_decoded_options_count, save_decoded_options); >> >> diff --git a/gcc/toplev.h b/gcc/toplev.h >> index d6c316962b0..627312f85f5 100644 >> --- a/gcc/toplev.h >> +++ b/gcc/toplev.h >> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see >> /* Decoded options, and number of such options. */ >> extern struct cl_decoded_option *save_decoded_options; >> extern unsigned int save_decoded_options_count; >> +extern auto_vec save_opt_decoded_options; >> >> class timer; >> >> -- >> 2.28.0 >>