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 427D73858D37 for ; Tue, 27 Oct 2020 11:13:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 427D73858D37 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 DCAB213D5; Tue, 27 Oct 2020 04:13:23 -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 313CA3F66E; Tue, 27 Oct 2020 04:13:23 -0700 (PDT) From: Richard Sandiford To: Hongtao Liu via Gcc-patches Mail-Followup-To: Hongtao Liu via Gcc-patches , Vladimir Makarov , Hongtao Liu , dcb314@hotmail.com, richard.sandiford@arm.com Cc: Vladimir Makarov , Hongtao Liu , dcb314@hotmail.com Subject: Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint. References: Date: Tue, 27 Oct 2020 11:13:21 +0000 In-Reply-To: (Hongtao Liu via Gcc-patches's message of "Tue, 27 Oct 2020 14:53:16 +0800") 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=-6.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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: Tue, 27 Oct 2020 11:13:25 -0000 Hongtao Liu via Gcc-patches writes: > Hi: > For inline asm, there could be an operand like (not (mem:)), it's > not a valid operand for normal memory constraint. > Bootstrap is ok, regression test is ok for make check > RUNTESTFLAGS="--target_board='unix{-m32,}'" > > gcc/ChangeLog > PR target/97540 > * ira.c: (ira_setup_alts): Extract memory from operand only > for special memory constraint. > * recog.c (asm_operand_ok): Ditto. > * lra-constraints.c (process_alt_operands): MEM_P is > required for normal memory constraint. > > gcc/testsuite/ChangeLog > * gcc.target/i386/pr97540.c: New test. Sorry to stick my oar in, but I think we should reconsider the bcst_mem_operand approach. It seems like these patches (and the previous one) are fighting against the principle that operands cannot be arbitrary expressions. This kind of thing was attempted long ago (even before my time!) for SIGN_EXTEND on MIPS. It ended up causing more problems than it solved and in the end it had to be taken out. I'm worried that we might end up going through the same cycle again. Also, this LRA code is extremely performance-sensitive in terms of compile time: it's often at the top or near the top of the profile. So adding calls to new functions like extract_mem_from_operand for a fairly niche case probably isn't a good trade-off. I think we should instead find a nice(?) syntax for generating separate patterns for the two bcst_vector_operand alternatives from a single .md pattern. That would fit the existing model much more closely. (To be clear, I'm not saying the existing model is perfect. I just think a change along these lines is more fundamental than it might look, and would need changes outside the register allocators to work reliably.) FWIW, in: (define_insn "*3" [(set (match_operand:VI_AVX2 0 "register_operand" "=x,v") (plusminus:VI_AVX2 (match_operand:VI_AVX2 1 "bcst_vector_operand" "0,v") (match_operand:VI_AVX2 2 "bcst_vector_operand" "xBm,vmBr")))] we can assume that any bcst_mem_operand will be first. Allowing it as operand 2 (as the constraints do) creates non-canonical RTL. So this at least is one case in which I think the bcst_mem_operand version has to be a separate .md construct. Sorry for not noticing or speaking up earlier. I realise it's extremely unhelpful to get this kind of comment after you've done so much work. :-( Thanks, Richard