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 9645438460A2 for ; Mon, 7 Sep 2020 20:21:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9645438460A2 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com 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 442411045; Mon, 7 Sep 2020 13:21:01 -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 C48BE3F68F; Mon, 7 Sep 2020 13:21:00 -0700 (PDT) From: Richard Sandiford To: "Qian\, Jianhua" Mail-Followup-To: "Qian\, Jianhua" , "gcc\@gcc.gnu.org" , richard.sandiford@arm.com Cc: "gcc\@gcc.gnu.org" Subject: Re: A problem with one instruction multiple latencies and pipelines References: Date: Mon, 07 Sep 2020 21:20:59 +0100 In-Reply-To: (Jianhua Qian's message of "Mon, 7 Sep 2020 06:08:50 +0000") 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=-7.5 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@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Sep 2020 20:21:04 -0000 "Qian, Jianhua" writes: > Hi > > I'm adding a new machine model. I have a problem when writing the "define= _insn_reservation" for instruction scheduling. > How to write the "define_insn_reservation" for one instruction that there= are different latencies and pipelines according to parameter. > > For example, the ADD (shifted register) instruction in a64fx > > Instruction Option Latency Pipe= line > ADD (shifted register) =3D 0 1 EX*= | EAG* > =3D [1-4] && =3DLSL 1+1 (EXA= + EXA) | (EXB + EXB) > 2+1 (EXA += EXA) | (EXB + EXB) > > In aarch64.md ADD (shifted register) instruction is defined as following= =EF=BC=8E > (define_insn "*add__" > [(set (match_operand:GPI 0 "register_operand" "=3Dr") > (plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r") > (match_operand:QI 2 "aarch64_shift_imm_" "n")) > (match_operand:GPI 3 "register_operand" "r")))] > "" > "add\\t%0, %3, %1, %2" > [(set_attr "type" "alu_shift_imm")] > ) > > It could not be distinguished by the type "alu_shift_imm" when writing "d= efine_insn_reservation" for ADD (shifted register).=20 > What should I do? This is just personal opinion, but in general (from the point of view of a new port, or a new subport like SVE), I think the best approach to handling the "type" attribute is to start with the coarsest classification that makes sense, then split these relatively coarse types up whenever there's a specific need. When taking that approach, it's OK (and perhaps even a good sign) for an existing type to sometimes be too coarse for a new CPU. So thanks for asking about this, and please don't feel constrained by the existing "type" classification. IMO we should split existing types wherever that makes sense for new CPUs. In a situation like this, I guess it would make sense to treat alu_shift_imm as a conservative general classification and add alu_shift_imm1to4 (say) as a new type. I'd recommend a two-stage approach, with each stage as a separate patch for ease of review: (1) Add the new type to the definition of the "type" attribute and run: sed -i s/alu_shift_imm/alu_shift_imm1to4,alu_shift_imm/g on all the config/arm and config/aarch64 tuning descriptions. This is a purely mechanical replacement and could be tested in the normal way (i.e. there would be no need to do special testing to make sure that existing descriptions are unaffected: the change is mechanical enough not to need that). (2) Make the define_insns use the new type where appropriate. Like Richard says, this would be handled by splitting the single define_insn alternative above into two separate alternatives. It's also possible to something like: (set (attr "type") (if_then_else (match_operand ...) (const_string "...") (const_string "..."))) which localises the change to the attribute definition. If we end up needing that construct several times, it would also be possible to add a new attribute, e.g.: (define_attr "shift_imm_type" "none,alu_shift_op2") and then change the default value of the type attribute from: (const_string "untyped") to: (cond [(eq_attr "shift_imm_type" "alu_shift_op2") (if_then_else (match_operand 2 ...) (const_string "...") (const_string "..."))] (const_string "untyped")) define_insns like the above could then use: (set_attr "shift_imm_type" "alu_shift_op2") instead of setting "type" directly. That's definitely more complicated, but it can help to reduce cut-&-paste. Thanks, Richard