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 3E8BC3858418 for ; Wed, 13 Jul 2022 08:10:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E8BC3858418 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 411F11424; Wed, 13 Jul 2022 01:10:28 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.37]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 37C403F792; Wed, 13 Jul 2022 01:10:27 -0700 (PDT) From: Richard Sandiford To: Richard Biener via Gcc-patches Mail-Followup-To: Richard Biener via Gcc-patches , Andrew Carlotti , Richard Biener , richard.sandiford@arm.com Subject: Re: [PATCH v2 1/2] aarch64: Don't return invalid GIMPLE assign statements References: Date: Wed, 13 Jul 2022 09:10:25 +0100 In-Reply-To: (Richard Biener via Gcc-patches's message of "Wed, 13 Jul 2022 09:41:52 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-54.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Wed, 13 Jul 2022 08:10:30 -0000 Richard Biener via Gcc-patches writes: > On Tue, Jul 12, 2022 at 4:38 PM Andrew Carlotti wrote: >> >> aarch64_general_gimple_fold_builtin doesn't check whether the LHS of a >> function call is null before converting it to an assign statement. To avoid >> returning an invalid GIMPLE statement in this case, we instead assign the >> expression result to a new (unused) variable. >> >> This change only affects code that: >> 1) Calls an intrinsic function that has no side effects; >> 2) Does not use or store the value returned by the intrinsic; >> 3) Uses parameters that prevent the front-end eliminating the call prior to >> gimplification. >> >> The ICE is unlikely to have occurred in the wild, as it relies on the presence >> of a redundant intrinsic call. > > Other targets usually simply refrain from folding intrinsic calls with no LHS. > Another option is to just drop it on the floor if it does not have any > side-effects which for the gimple_fold_builtin hook means folding it to > a GIMPLE_NOP (gimple_build_nop ()). Sorry, I just pushed the patch before seeing this. I guess the problem with refraining from folding calls with no lhs is that it has to be done on a per-function basis. (E.g. stores should still be folded.) It then becomes something that we need to remember for each individual call. E.g. ix86_gimple_fold_builtin seems to have three different pieces of code for handling null lhses, even with its heavy use of gotos. So a nice thing about the current patch is that it handles all this in one place only. Thanks, Richard >> gcc/ChangeLog: >> >> * config/aarch64/aarch64-builtins.cc >> (aarch64_general_gimple_fold_builtin): Add fixup for invalid GIMPLE. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c: New test. >> >> --- >> >> diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc >> index e0a741ac663188713e21f457affa57217d074783..5753988a9964967c27a03aca5fddb9025fd8ed6e 100644 >> --- a/gcc/config/aarch64/aarch64-builtins.cc >> +++ b/gcc/config/aarch64/aarch64-builtins.cc >> @@ -3022,6 +3022,16 @@ aarch64_general_gimple_fold_builtin (unsigned int fcode, gcall *stmt, >> default: >> break; >> } >> + >> + /* GIMPLE assign statements (unlike calls) require a non-null lhs. If we >> + created an assign statement with a null lhs, then fix this by assigning >> + to a new (and subsequently unused) variable. */ >> + if (new_stmt && is_gimple_assign (new_stmt) && !gimple_assign_lhs (new_stmt)) >> + { >> + tree new_lhs = make_ssa_name (gimple_call_return_type (stmt)); >> + gimple_assign_set_lhs (new_stmt, new_lhs); >> + } >> + >> return new_stmt; >> } >> >> diff --git a/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c >> new file mode 100644 >> index 0000000000000000000000000000000000000000..345307456b175307f5cb22de5e59cfc6254f2737 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c >> @@ -0,0 +1,9 @@ >> +/* { dg-do compile { target { aarch64*-*-* } } } */ >> + >> +#include >> + >> +int8_t *bar(); >> + >> +void foo() { >> + __builtin_aarch64_ld1v16qi(bar()); >> +}