From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97429 invoked by alias); 24 Dec 2019 16:25:09 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 97143 invoked by uid 89); 24 Dec 2019 16:24:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=meissner, Meissner, gen_rtx_PLUS, xexp X-HELO: gate.crashing.org Received: from gate.crashing.org (HELO gate.crashing.org) (63.228.1.57) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 24 Dec 2019 16:24:58 +0000 Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id xBOGOuoY006056; Tue, 24 Dec 2019 10:24:56 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id xBOGOtvf006053; Tue, 24 Dec 2019 10:24:55 -0600 Date: Wed, 25 Dec 2019 06:41:00 -0000 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, David Edelsohn Subject: Re: [PATCH] V11 patch #5 of 15, Optimize vec_extract of a vector in memory with a PC-relative address Message-ID: <20191224162455.GI4505@gate.crashing.org> References: <20191220231507.GA18386@ibm-toto.the-meissners.org> <20191220235553.GE28993@ibm-toto.the-meissners.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191220235553.GE28993@ibm-toto.the-meissners.org> User-Agent: Mutt/1.4.2.3i X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg01557.txt.bz2 Hi! On Fri, Dec 20, 2019 at 06:55:53PM -0500, Michael Meissner wrote: > * config/rs6000/rs6000.c (rs6000_reg_to_addr_mask): New helper > function to identify the address mask of a hard register. Do this as a separate patch please. That refactoring is pre-approved. Please explain in the function comment what an "address mask" is. Or better yet, don't call it a "mask", it isn't a mask? Also various of the names here still have "reload" in it, which doesn't really make much sense. rs6000_mode_to_addressing_flags? And a reg_to for this new one? Something like that. > + /* For references to local static variables, try to fold a constant offset > + into the address. */ > + else if (pcrel_local_address (addr, Pmode) && CONST_INT_P (element_offset)) > + { > + if (GET_CODE (addr) == CONST) > + addr = XEXP (addr, 0); > + > + if (GET_CODE (addr) == PLUS) > + { > + rtx op0 = XEXP (addr, 0); > + rtx op1 = XEXP (addr, 1); > + if (CONST_INT_P (op1)) > + { > + HOST_WIDE_INT offset > + = INTVAL (XEXP (addr, 1)) + INTVAL (element_offset); > + > + if (offset == 0) > + new_addr = op0; > + > + else if (SIGNED_INTEGER_34BIT_P (offset)) > + { > + rtx plus = gen_rtx_PLUS (Pmode, op0, GEN_INT (offset)); > + new_addr = gen_rtx_CONST (Pmode, plus); > + } > + > + else > + { > + emit_move_insn (base_tmp, addr); > + new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset); > + } > + } > + else > + { > + emit_move_insn (base_tmp, addr); > + new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset); > + } > + } > + > + else > + { > + rtx plus = gen_rtx_PLUS (Pmode, addr, element_offset); > + new_addr = gen_rtx_CONST (Pmode, plus); > + } > + } This adds four new if's and four new else's, indented three deep. Please write it as a separate function? Something like "pcrel_adjust_address", adding an extra offset? Or not pcrel perhaps, you can do other addresses in the same routine? Either way, adjust the address is useful more often than for extracts, and is a much more general thing to do. So please split that out from the existing code, as a separate patch again, and then add to that? > + /* If we have a PC-relative address, check if offsetable loads are > + allowed. */ > + else if (pcrel_local_address (new_addr, Pmode)) > + { > + addr_mask_type addr_mask > + = rs6000_reg_to_addr_mask (scalar_reg, scalar_mode); > + > + valid_addr_p = (addr_mask & RELOAD_REG_OFFSET) != 0; > + } That comment could be better, too? (And two letters "t" in offsettable). Thanks, Segher