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 C1D823858D35 for ; Mon, 3 Aug 2020 13:55:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C1D823858D35 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 7E15730E; Mon, 3 Aug 2020 06:55:28 -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 E1EC33F718; Mon, 3 Aug 2020 06:55:27 -0700 (PDT) From: Richard Sandiford To: xiezhiheng Mail-Followup-To: xiezhiheng , Richard Biener , "gcc-patches\@gcc.gnu.org" , richard.sandiford@arm.com Cc: Richard Biener , "gcc-patches\@gcc.gnu.org" Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions emitted at -O3 References: <014c7f5ef7874db4ae98470c298b1f9b@huawei.com> <6ce35b1fc9c74013bac5066b21754d6c@huawei.com> <19e442ee9ffe445ba4b09ec569f5257f@huawei.com> <61bd22ee42114c6a823375a2b372decd@huawei.com> <6522d772e22a4453a6bd80026b3810d4@huawei.com> <3c4244b3625e4f7eb23f93c564ea687f@huawei.com> Date: Mon, 03 Aug 2020 14:55:26 +0100 In-Reply-To: <3c4244b3625e4f7eb23f93c564ea687f@huawei.com> (xiezhiheng@huawei.com's message of "Mon, 3 Aug 2020 02:21:56 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_NUMSUBJECT, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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, 03 Aug 2020 13:55:30 -0000 xiezhiheng writes: >> -----Original Message----- >> From: Richard Sandiford [mailto:richard.sandiford@arm.com] >> Sent: Friday, July 31, 2020 5:03 PM >> To: xiezhiheng >> Cc: Richard Biener ; gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions >> emitted at -O3 >>=20 >> xiezhiheng writes: >> >> -----Original Message----- >> >> From: Richard Sandiford [mailto:richard.sandiford@arm.com] >> >> Sent: Friday, July 17, 2020 5:04 PM >> >> To: xiezhiheng >> >> Cc: Richard Biener ; >> gcc-patches@gcc.gnu.org >> >> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions >> >> emitted at -O3 >> >> >> > >> > Cut... >> > >> >> >> >> Thanks, pushed to master. >> >> >> >> Richard >> > >> > And I have finished the second part. >> > >> > In function aarch64_general_add_builtin, I add an argument ATTRS to >> > pass attributes for each built-in function. >> > >> > And some new functions are added: >> > aarch64_call_properties: return flags for each built-in function based >> > on command-line options. When the built-in function handles >> > floating-points, add FLAG_FP flag. >> > >> > aarch64_modifies_global_state_p: True if the function would modify >> > global states. >> > >> > aarch64_reads_global_state_p: True if the function would read >> > global states. >> > >> > aarch64_could_trap_p: True if the function would raise a signal. >> > >> > aarch64_add_attribute: Add attributes in ATTRS. >> > >> > aarch64_get_attributes: return attributes for each built-in functons >> > based on flags and command-line options. >> > >> > In function aarch64_init_simd_builtins, attributes are get by flags >> > and pass them to function aarch64_general_add_builtin. >> > >> > >> > Bootstrap is tested OK on aarch64 Linux platform, but regression >> > FAIL one test case ---- pr93423.f90. >> > However, I found that this test case would fail randomly in trunk. >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D93423 >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D96041 >> > Some PRs have tracked it. After my patch, this test case would >> > always fail. I guess the syntax errors in fortran crash some structur= es >> > result in illegal memory access but I can't find what exactly it is. >> > But I think my patch should have no influence on it. >>=20 >> Yeah, I agree. And FWIW, I didn't see this in my testing. >>=20 >> I've pushed the patch with one trivial change: to remove the =E2=80=9Can= d=E2=80=9D >> before =E2=80=9CCODE=E2=80=9D in: >>=20 >> > /* Wrapper around add_builtin_function. NAME is the name of the >> built-in >> > function, TYPE is the function type, and CODE is the function subc= ode >> > - (relative to AARCH64_BUILTIN_GENERAL). */ >> > + (relative to AARCH64_BUILTIN_GENERAL), and ATTRS is the function >> > + attributes. */ >>=20 >> BTW, one thing to be careful of in future is that not all FP intrinsics >> raise FP exceptions. So while: >>=20 >> > + switch (d->mode) >> > + { >> > + /* Floating-point. */ >> > + case E_BFmode: >> > + case E_V4BFmode: >> > + case E_V8BFmode: >> > + case E_HFmode: >> > + case E_V4HFmode: >> > + case E_V8HFmode: >> > + case E_SFmode: >> > + case E_V2SFmode: >> > + case E_V4SFmode: >> > + case E_DFmode: >> > + case E_V1DFmode: >> > + case E_V2DFmode: >> > + flags |=3D FLAG_FP; >> > + break; >> > + >> > + default: >> > + break; >> > + } >>=20 >> is a good, conservatively-correct default, we might need an additional >> flag to suppress it for certain intrinsics. >>=20 > > I agree. > >> I've just realised that the code above could have used FLOAT_MODE_P, >> but I didn't think of that before pushing the patch :-) >>=20 > > Sorry, I should have used it. And I prepare a patch to use FLOAT_MODE_P > macro and add a flag FLAG_SUPPRESS_FP_EXCEPTIONS to suppress > FLAG_RAISE_FP_EXCEPTIONS for certain intrinsics in future. The same thing is true for reading FPCR as well, so I think the flag should suppress the FLOAT_MODE_P check, instead of fixing up the flags afterwards. I'm struggling to think of a good name though. How about adding FLAG_AUTO_FP and making the FLOAT_MODE_P check dependent on FLAG_AUTO_FP being set? We could leave FLAG_AUTO_FP out of FLAG_ALL, since FLAG_ALL already includes FLAG_FP. Including it in FLAG_ALL wouldn't do no any harm though. Thanks, Richard