From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118137 invoked by alias); 19 Aug 2019 17:47:53 -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 118129 invoked by uid 89); 19 Aug 2019 17:47:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Due 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; Mon, 19 Aug 2019 17:47:52 +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 x7JHlnTX003667; Mon, 19 Aug 2019 12:47:49 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id x7JHlnmK003666; Mon, 19 Aug 2019 12:47:49 -0500 Date: Mon, 19 Aug 2019 19:15:00 -0000 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, David Edelsohn , Alan Modra Subject: Re: [PATCH], Patch #2 of 10, Add RTL prefixed attribute Message-ID: <20190819174748.GN31406@gate.crashing.org> References: <20190814205732.GA11956@ibm-toto.the-meissners.org> <20190814215913.GB16578@ibm-toto.the-meissners.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190814215913.GB16578@ibm-toto.the-meissners.org> User-Agent: Mutt/1.4.2.3i X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg01331.txt.bz2 Hi Mike, Some comments on this patch: On Wed, Aug 14, 2019 at 05:59:13PM -0400, Michael Meissner wrote: > Due to some of the existing load and store insns not using the traditional > operands[0] and operands[1], the functions that test whether an insn is > prefixed only use the insn and not the operands directly. Both the "update" and the "indexed" attributes have no problem with this: the insns that have the problem set the attribute value directly. This is mainly all the various update insns, but there are a bunch more, and they all need different settings for their attributes. > * config/rs6000/rs6000.c (rs6000_emit_move): Add support for > loading up pc-relatve addresses. (typo btw) > +void > +rs6000_final_prescan_insn (rtx_insn *insn) > +{ > + next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO); > + return; > +} Don't say "return;" at the end of a function please. > +void > +rs6000_asm_output_opcode (FILE *stream, const char *) > +{ > + if (next_insn_prefixed_p) > + { > + next_insn_prefixed_p = false; > + fprintf (stream, "p"); > + } You don't need to clear the flag here; the next call to rs6000_final_prescan_insn will. > +#define FINAL_PRESCAN_INSN(INSN, OPERANDS, NOPERANDS) \ > +do \ > + { \ > + if (TARGET_PREFIXED_ADDR) \ > + rs6000_final_prescan_insn (INSN); \ > + } \ > +while (0) Either have the function only do what it needs to for prefixed, and call it something with prefixed in the name, or put the TARGET_PREFIXED_ADDR test in the function itself. > +;; Load up a pc-relative address. ASM_OUTPUT_OPCODE will emit the initial "p". > +(define_insn "*pcrel_addr" > + [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r") > + (match_operand:DI 1 "pcrel_address"))] > + "TARGET_PCREL" > + "la %0,%a1" > + [(set_attr "prefixed" "yes")]) (use P for addresses please) > +;; Load up a pc-relative address to an external symbol. If the symbol and the > +;; program are both defined in the main program, the linker will optimize this > +;; to a PADDI. Otherwise, it will create a GOT address that is relocated by > +;; the dynamic linker and loaded up. > +(define_insn "*pcrel_ext_addr" > + [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r") > + (match_operand:DI 1 "pcrel_external_address"))] > + "TARGET_PCREL" > + "ld %0,%a1" > + [(set_attr "prefixed" "yes")]) pld does an indirection more than pla does, but this is not clear at all from the RTL, from the predicate names. All this is *before* the linker has done its thing, so pcrel_external_address is really some GOT memory, so it should have that in its name. Segher