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 52FE43858C78 for ; Tue, 12 Dec 2023 10:12:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 52FE43858C78 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 52FE43858C78 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=1702375943; cv=none; b=ii+joEpfgrXHprWKLj37c8KhSbAyFN8PdaRwKug1a3LjFi4shAORuHIybYSd3GL82tvdGfvpMB3mmZvAhOUHt4lTKoef5qLcXK42q95KWMhuZWmnheQIl3+g9fLVG+1bWzyIFQIzR5Moh8bkdiusqHowDHEO95Fd7IEOAtJWHy0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702375943; c=relaxed/simple; bh=X2a9GzJlC4Gzu0ZpexnuEpjgMxy8lA6SsfnvseyNtOM=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=BYir7STEeT1gm4RYWK5+LStGbPkLGxt3O6a4SxRZAlv4NuakxL2WFvjYHI86BMeOddqnlPLAJimvV8iJ0YjubokKOwk3coVSYGrRO1F0J5Z+1ygkZnWTRZ2yAklBTLmV1A44Tg2Mikbbmr5Q+oKS+fQSaSS+umHv1Vk64VrMAkM= 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 69ACB143D; Tue, 12 Dec 2023 02:13:07 -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 699703F762; Tue, 12 Dec 2023 02:12:20 -0800 (PST) From: Richard Sandiford To: Andrew Pinski Mail-Followup-To: Andrew Pinski ,Jeff Law , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Treat "p" in asms as addressing VOIDmode References: Date: Tue, 12 Dec 2023 10:12:19 +0000 In-Reply-To: (Andrew Pinski's message of "Mon, 11 Dec 2023 23:14:27 -0800") 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=-21.8 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 List-Id: Andrew Pinski writes: > On Mon, Dec 11, 2023 at 11:46=E2=80=AFAM Richard Sandiford > wrote: >> >> Jeff Law writes: >> > On 11/27/23 05:12, Richard Sandiford wrote: >> >> 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 =3D result || address_operand (op, VOIDmode); >> >> break; >> >> >> >> After RA it deferred to constrain_operands, which used the mode >> >> of the operand: >> >> >> >> if ((GET_MODE (op) =3D=3D VOIDmode >> >> || SCALAR_INT_MODE_P (GET_MODE (op))) >> >> && (strict <=3D 0 >> >> || (strict_memory_address_p >> >> (recog_data.operand_mode[opno], op)))) >> >> win =3D 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] >> >> =3D 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 th= an >> >> the mode of the address itself. For example, vax has: >> >> >> >> (define_insn "*movaddr" >> >> [(set (match_operand:SI 0 "nonimmediate_operand" "=3Dg") >> >> (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 was initially a bit counter-intuitive, my first reaction was that= a >> > wildcard mode is more general. And that's true, but it necessarily >> > means the addresses accepted are more restrictive because any mode is >> > allowed. >> >> Right. I should probably have a conservative, common subset. >> >> >> 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 f= or >> >> asms but continuing to use the operand mode for .md insns, so as not >> >> to break ports that still use reload. >> > Sadly I didn't get as far as I would have liked in removing reload, >> > though we did get a handful of ports converted this cycle >> > >> >> >> >> 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_op= erands >> >> for asms. >> >> >> >> gcc/testsuite/ >> >> * gcc.target/aarch64/prfm_imm_offset_2.c: New test. >> > It all seems a bit hackish. I don't think ports have had much success >> > using 'p' through the decades. I think I generally ended up having to >> > go with distinct constraints rather than relying on 'p'. >> > >> > OK for the trunk, but ewww. >> >> Thanks, pushed. And yeah, eww is fair. I'd be happy for this to become >> an unconditional VOIDmode once reload is removed. > > > The testcase prfm_imm_offset_2.c fails with a compile error: > /home/apinski/src/upstream-full-cross/gcc/gcc/testsuite/gcc.target/aarch6= 4/prfm_imm_offset_2.c: > In function 'f': > /home/apinski/src/upstream-full-cross/gcc/gcc/testsuite/gcc.target/aarch6= 4/prfm_imm_offset_2.c:1:46: > error: expected ')' before ':' token > > Most likely you need to add `/* { dg-options "" } */` to the beginning > of the file so it does not compile with `-ansi -pedantic-errors` > options which are default for the testsuite. Yeah, sorry. I fixed this at least twice in my tree, but I was having to carry the patch around in multiple branches, and must have cherry-picked from an unfixed branch. Here's what I pushed. Sorry for the breakage. Richard gcc/testsuite/ * gcc.target/aarch64/prfm_imm_offset_2.c: Add dg-options. --- gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c b/gcc/tes= tsuite/gcc.target/aarch64/prfm_imm_offset_2.c index 2dd695157f2..04e3fb72c45 100644 --- a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c +++ b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c @@ -1 +1,2 @@ +/* { dg-options "-O2" } */ void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); } --=20 2.25.1