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 0C8C73851C2E for ; Fri, 19 Mar 2021 15:42:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0C8C73851C2E 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 790AF31B; Fri, 19 Mar 2021 08:42:44 -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 030FF3F718; Fri, 19 Mar 2021 08:42:43 -0700 (PDT) From: Richard Sandiford To: Vladimir Makarov via Gcc-patches Mail-Followup-To: Vladimir Makarov via Gcc-patches , Vladimir Makarov , richard.sandiford@arm.com Subject: Re: [PATCH] [PR99581] Define relaxed memory and use it for aarch64 References: <0c334fcb-6847-22b1-1bd2-4c6309f328bc@redhat.com> Date: Fri, 19 Mar 2021 15:42:42 +0000 In-Reply-To: <0c334fcb-6847-22b1-1bd2-4c6309f328bc@redhat.com> (Vladimir Makarov via Gcc-patches's message of "Fri, 19 Mar 2021 10:21:20 -0400") 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=-6.0 required=5.0 tests=BAYES_00, KAM_ASCII_DIVIDERS, 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: Fri, 19 Mar 2021 15:42:46 -0000 Vladimir Makarov via Gcc-patches writes: > The following patch solves P1 PR99581 > > =C2=A0=C2=A0=C2=A0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D99581 > > The patch was successfully tested and bootstrapped on x86-64, ppc64le,=20 > aarch64. > > Is it ok for the trunk? As I mentioned in bugzilla though, the motivation behind this seems to be that "o" shouldn't need to check for a valid memory address, even though "m" and "V" do: -------------------------------------------------------------------------- (define_memory_constraint "TARGET_MEM_CONSTRAINT" "Matches any valid memory." (and (match_code "mem") (match_test "memory_address_addr_space_p (GET_MODE (op), XEXP (op, 0= ), MEM_ADDR_SPACE (op))"))) (define_memory_constraint "o" "Matches an offsettable memory reference." (and (match_code "mem") (match_test "offsettable_nonstrict_memref_p (op)"))) ;; "V" matches TARGET_MEM_CONSTRAINTs that are rejected by "o". ;; This means that it is not a memory constraint in the usual sense, ;; since reloading the address into a base register would make the ;; address offsettable. (define_constraint "V" "Matches a non-offsettable memory reference." (and (match_code "mem") (match_test "memory_address_addr_space_p (GET_MODE (op), XEXP (op, 0= ), MEM_ADDR_SPACE (op))") (not (match_test "offsettable_nonstrict_memref_p (op)")))) -------------------------------------------------------------------------- If the model behind this is that memory constraints should be able to assume basic validity (in the targetm.legitimate_address sense) then it seems like "m" should just be: -------------------------------------------------------------------------- (define_memory_constraint "TARGET_MEM_CONSTRAINT" "Matches any valid memory." (match_code "mem")) -------------------------------------------------------------------------- But it seems to be long-established precedent that "m" isn't just that. So I'm reluctant to go down this path if there's no clear reason why "m" has to have the check but "o" doesn't. I think we're also opening the possiblity that we'll need a define_special_relaxed_memory_constraint in future. The reason why my LRA patch seemed OK to me was that we'd already done the same thing for address constraints: /* Target hooks sometimes don't treat extra-constraint addresses as legitimate address_operands, so handle them specially. */ if (insn_extra_address_constraint (cn) && satisfies_address_constraint_p (&ad, cn)) return change_p; which ultimately came from: https://gcc.gnu.org/g:2c62cbaaf13c78f10657e91efdb8352dc8898b0d and so is also long-standing precedent as this point. So I think we should also have a model for why normal address constraints can do this but normal memory constraints can't. I'm not trying to reject the patch as such. I just think we need to have a clearer picture first. Thanks, Richard