From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 38B72386102B for ; Mon, 20 Jul 2020 14:03:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 38B72386102B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EA16BD6E; Mon, 20 Jul 2020 07:03:16 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C7E4C3F718; Mon, 20 Jul 2020 07:03:15 -0700 (PDT) From: Richard Sandiford To: Hu Jiangping Mail-Followup-To: Hu Jiangping , , richard.sandiford@arm.com Cc: Subject: Re: [PATCH] target: fix default value checking of x_str_align_functions in aarch64.c References: <20200714075518.4653-1-hujiangping@cn.fujitsu.com> Date: Mon, 20 Jul 2020 15:03:14 +0100 In-Reply-To: <20200714075518.4653-1-hujiangping@cn.fujitsu.com> (Hu Jiangping's message of "Tue, 14 Jul 2020 15:55:18 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, 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, 20 Jul 2020 14:03:18 -0000 Hu Jiangping writes: > Hi, > > This patch deal with the -falign-X=3D0 options. According to man pages, > if zero is specified, a machine-dependent default value should be used. > But in fact, zero was used in internal process, it is inconsistent. > > Tested on aarch64-linux cross compiler, Is that OK? > > BTW, the similar problems exists in other target sources. > I can submit them all in another patch if needed, > but I can test on i386 target only. Sorry for the slow response on this. Like you say, it seems to be a pretty pervasive problem. In fact I couldn't see anywhere that actually treated -falign-foo=3D0 as anything other than -falign-foo=3D1. Technically using an alignment of one for zero is within what the documentation allows, but not in a useful way. The documentation also isn't clear about whether: -falign-loops=3D0:8 (=E2=80=9Calign to whatever you think is best, but don't skip more than 8 b= ytes=E2=80=9D) is supposed to be valid. The implication is that it's OK, but in practice it doesn't work. If there isn't anywhere that handles zero in the way that the documentation implies (i.e. with -falign-loops=3D0 being equivalent to -falign-loops) then maybe we should instead change the documentation to match the actual behaviour. If instead this is a regression from previous compilers, then I guess we should fix it. But I think it would be good to have a helper function that tests whether the default should be used for a given x_flag_align_foo and x_str_align_foo pair. That we we could reuse it in other targets and would have only one place to update. (For example, we might decide to use parse_and_check_align_values rather than strcmp.) Don't know whether anyone else has any thoughts about the best fix. Thanks, and sorry again for the slow reply. Richard > > Regards! > Hujp > > --- > gcc/config/aarch64/aarch64.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 17dbe673978..697ac676f4d 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -14221,11 +14221,14 @@ aarch64_override_options_after_change_1 (struct= gcc_options *opts) > alignment to what the target wants. */ > if (!opts->x_optimize_size) > { > - if (opts->x_flag_align_loops && !opts->x_str_align_loops) > + if ((opts->x_flag_align_loops && !opts->x_str_align_loops) > + || (opts->x_str_align_loops && strcmp(opts->x_str_align_loops, "= 0") =3D=3D 0)) > opts->x_str_align_loops =3D aarch64_tune_params.loop_align; > - if (opts->x_flag_align_jumps && !opts->x_str_align_jumps) > + if ((opts->x_flag_align_jumps && !opts->x_str_align_jumps) > + || (opts->x_str_align_jumps && strcmp(opts->x_str_align_jumps, "= 0") =3D=3D 0)) > opts->x_str_align_jumps =3D aarch64_tune_params.jump_align; > - if (opts->x_flag_align_functions && !opts->x_str_align_functions) > + if ((opts->x_flag_align_functions && !opts->x_str_align_functions) > + || (opts->x_str_align_functions && strcmp(opts->x_str_align_func= tions, "0") =3D=3D 0)) > opts->x_str_align_functions =3D aarch64_tune_params.function_align; > }