From patchwork Mon Dec 11 15:23:02 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 81911 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A8E083858013 for ; Mon, 11 Dec 2023 15:23:21 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 4D6C33858D28 for ; Mon, 11 Dec 2023 15:23:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4D6C33858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4D6C33858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702308186; cv=none; b=MZjiX/6AADjkRgd3Mt/TyK+cny08yz0rf6c/MCxPWwwVCtkPERe4gh+xBnRqI47/5PXVBGxSmAR8/FcU84RWuettAFJ2YcUXLiRFblo4jvX5kEaC0wSTo1XQaYPNFGHdDj0Cv2AacSG676yZ722hJAd2jAiDoWot8gbjw79DOyw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702308186; c=relaxed/simple; bh=oPm5khI3WyCYJd/OBvkx1aO4a8vOWUNYps+SIODZ35M=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=qWWyOLis8YUL/vWP4+UD/hsojXkHLpXf01udUBbdRTTkzx5dPtd0L5CH1ymicLcxEPRFNInkoQaEPnpeqd9d90hV7hGE3Sm7szlH0g+vw4hEpRTtaDf1fwi9z2h7XZuOSPdLUHCk6nAXZ+ey4cdkloodJi47DhQSdTetkzTFWec= ARC-Authentication-Results: i=1; server2.sourceware.org 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 5AC07FEC for ; Mon, 11 Dec 2023 07:23:50 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6D0F43F738 for ; Mon, 11 Dec 2023 07:23:03 -0800 (PST) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Subject: Ping: [PATCH] Treat "p" in asms as addressing VOIDmode References: Date: Mon, 11 Dec 2023 15:23:02 +0000 In-Reply-To: (Richard Sandiford's message of "Mon, 27 Nov 2023 12:12:11 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Spam-Status: No, score=-21.9 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Ping --- check_asm_operands was inconsistent about how it handled "p" after RA compared to before RA. Before RA it tested the address with a void (unknown) memory mode: case CT_ADDRESS: /* Every address operand can be reloaded to fit. */ result = result || address_operand (op, VOIDmode); break; After RA it deferred to constrain_operands, which used the mode of the operand: if ((GET_MODE (op) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (op))) && (strict <= 0 || (strict_memory_address_p (recog_data.operand_mode[opno], op)))) win = true; Using the mode of the operand matches reload's behaviour: else if (insn_extra_address_constraint (lookup_constraint (constraints[i]))) { address_operand_reloaded[i] = find_reloads_address (recog_data.operand_mode[i], (rtx*) 0, recog_data.operand[i], recog_data.operand_loc[i], i, operand_type[i], ind_levels, insn); It allowed the special predicate address_operand to be used, with the mode of the operand being the mode of the addressed memory, rather than the mode of the address itself. For example, vax has: (define_insn "*movaddr" [(set (match_operand:SI 0 "nonimmediate_operand" "=g") (match_operand:VAXfp 1 "address_operand" "p")) (clobber (reg:CC VAX_PSL_REGNUM))] "reload_completed" "mova %a1,%0") where operand 1 is an SImode expression that can address memory of mode VAXfp. GET_MODE (recog_data.operand[1]) is SImode (or VOIDmode), but recog_data.operand_mode[1] is mode. But AFAICT, ira and lra (like pre-reload check_asm_operands) do not do this, and instead pass VOIDmode. So I think this traditional use of address_operand is effectively an old-reload-only feature. And it seems like no modern port cares. I think ports have generally moved to using different address constraints instead, rather than relying on "p" with different operand modes. Target-specific address constraints post-date the code above. The big advantage of using different constraints is that it works for asms too. And that (to finally get to the point) is the problem fixed in this patch. For the aarch64 test: void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); } everything up to and including RA required the operand to be a valid VOIDmode address. But post-RA check_asm_operands and constrain_operands instead required it to be valid for recog_data.operand_mode[0]. Since asms have no syntax for specifying an operand mode that's separate from the operand itself, operand_mode[0] is simply Pmode (i.e. DImode). This meant that we required one mode before RA and a different mode after RA. On AArch64, VOIDmode is treated as a wildcard and so has a more conservative/restricted range than DImode. So if a post-RA pass tried to form a new address, it would use a laxer condition than the pre-RA passes. This happened with the late-combine pass that I posted in October: https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634166.html which in turn triggered an error from aarch64_print_operand_address. This patch takes the (hopefully) conservative fix of using VOIDmode for asms but continuing to use the operand mode for .md insns, so as not to break ports that still use reload. Fixing this made me realise that recog_level2 was doing duplicate work for asms after RA. Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? Richard gcc/ * recog.cc (constrain_operands): Pass VOIDmode to strict_memory_address_p for 'p' constraints in asms. * rtl-ssa/changes.cc (recog_level2): Skip redundant constrain_operands for asms. gcc/testsuite/ * gcc.target/aarch64/prfm_imm_offset_2.c: New test. --- gcc/recog.cc | 18 +++++++++++------- gcc/rtl-ssa/changes.cc | 4 +++- .../gcc.target/aarch64/prfm_imm_offset_2.c | 2 ++ 3 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c diff --git a/gcc/recog.cc b/gcc/recog.cc index eaab79c25d7..bff7be1aec1 100644 --- a/gcc/recog.cc +++ b/gcc/recog.cc @@ -3191,13 +3191,17 @@ constrain_operands (int strict, alternative_mask alternatives) strictly valid, i.e., that all pseudos requiring hard regs have gotten them. We also want to make sure we have a valid mode. */ - if ((GET_MODE (op) == VOIDmode - || SCALAR_INT_MODE_P (GET_MODE (op))) - && (strict <= 0 - || (strict_memory_address_p - (recog_data.operand_mode[opno], op)))) - win = true; - break; + { + auto mem_mode = (recog_data.is_asm + ? VOIDmode + : recog_data.operand_mode[opno]); + if ((GET_MODE (op) == VOIDmode + || SCALAR_INT_MODE_P (GET_MODE (op))) + && (strict <= 0 + || strict_memory_address_p (mem_mode, op))) + win = true; + break; + } /* No need to check general_operand again; it was done in insn-recog.cc. Well, except that reload diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc index 2f2d12d5f30..443d0575df5 100644 --- a/gcc/rtl-ssa/changes.cc +++ b/gcc/rtl-ssa/changes.cc @@ -986,8 +986,10 @@ recog_level2 (insn_change &change, add_regno_clobber_fn add_regno_clobber) pat = newpat; } + // check_asm_operands checks the constraints after RA, so we don't + // need to do it again. INSN_CODE (rtl) = icode; - if (reload_completed) + if (reload_completed && !asm_p) { extract_insn (rtl); if (!constrain_operands (1, get_preferred_alternatives (rtl))) diff --git a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c new file mode 100644 index 00000000000..04e3fb72c45 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c @@ -0,0 +1,2 @@ +/* { dg-options "-O2" } */ +void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); }