From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id E278B38618F9 for ; Sun, 29 Oct 2023 21:44:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E278B38618F9 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E278B38618F9 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698615898; cv=none; b=IfntrCt3A8awjvs6eY2dZd6g3pylYAJt3xtvWqyraa8I9lsPBXImmHPlyUH+2AbrnDEn3bqS0DoQJNG+ATuplPgsM/KkqZIE9cLWqfv+oW9ztzlw7nhMfZeAFF9gOtFdFrZNCPcbfSZRWhl9AI8w9IIL5zEc3kCgw4Ru/6jsuMQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698615898; c=relaxed/simple; bh=zN5kTctgJaWAxoGW7tvSA3+WdkoZ+HrOfaronJXvjtA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=ENyTepFcArgNgm7sdT/IWt6zmb5LisGhXTUH7hVMUnRaebDfg79WNN3zHAKjosJG22plZu64jSVsSp+TEIacf/NfzXmIxERtWDy96ThEZtNrkDSdVpu8zwEdYNdYIU6ErUJkE+lW/f+Q/RdJJKaCVrQ6lq6YSsSOKbNx2Nu1gZ4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1cacde97002so25152335ad.2 for ; Sun, 29 Oct 2023 14:44:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698615895; x=1699220695; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=dvOpnjK7y+LpSBkyMUK2QFW2/9TqpBOCU4tEXYlu31A=; b=fQWUfkQfXzBtSShetFH7e9wxnIu8QeKfRjngYrMdIGhxMjCawCOU9njKSNNUyOneYa RuhOIN9iG+cl0o7axWNsJ4p5NXeIWt9yt8kCQ+J9951nIUej1hNQzgvXf/Eey9V6rNJW 5E6/LoVO8kSfaY+EP8PSe59Mo4kVRLAfWLNiQ3t1vTlf9RrzqzCSdRY1zNwEi+4/3Nak 824/qvTLOY8dvMNeXmB5t4x4JEOFGRxzEF2wrCHiq8rbgqrGX2RuG05olqynjezo1n0X B0b/5VMqSNp3cdF9jYTiMG7Kc9UQRPzukmZ8tfVr37gginCysA1iGgBUomWXNT4WqPXJ wxdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698615895; x=1699220695; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dvOpnjK7y+LpSBkyMUK2QFW2/9TqpBOCU4tEXYlu31A=; b=vQ01DeIiIaVl14cdDL+8+w3WRgZcucqsoz8SQONULPUBZN4ixdVQLCo28URH4pxEjH DHzilpKEGfJPpx2tCRaM2t2+B1Y4VBAx0RYI9nBGI7mQjadjQnOxRESYq2g+GI6FtfWj RGn19fXwZfZaNabMI0emlaoL0877Nc5wi6BEdKCkw4WNIqO3TYCpV4P+guqAW2NXBFmA Jw5Tp29Nj8NhUxaAapgcbmAjZ17EMgKRQZUxrBrDBMlnncnQMi81YHHuLdiprU1Pd4D1 ftVUtK0VyHsgHNmUwExEc0XzoHize6B5iC0Wl3DASw/63xcSl+23Diz7UXr/LSXH71Dj ywtQ== X-Gm-Message-State: AOJu0YxiOK893qRqyuz10QzuGWNKBSbshY0YISk6pwbiqWVOmmcKVhH+ TcZA6csFl4oUvfJYaPdnUvk= X-Google-Smtp-Source: AGHT+IEGzAKreCL/2oo12bZ4EBaH52b/i0MYDSDSE0qdxBL8RM6p4MxqLEatIintBp5grlhoS6c43g== X-Received: by 2002:a17:902:ecd0:b0:1cc:2c78:65d9 with SMTP id a16-20020a170902ecd000b001cc2c7865d9mr4447619plh.25.1698615894576; Sun, 29 Oct 2023 14:44:54 -0700 (PDT) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id l15-20020a170902f68f00b001c73f3a9b88sm4919919plg.110.2023.10.29.14.44.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 29 Oct 2023 14:44:54 -0700 (PDT) Message-ID: Date: Sun, 29 Oct 2023 15:44:47 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/2] riscv: thead: Add support for the XTheadMemIdx ISA extension Content-Language: en-US To: Christoph Muellner , gcc-patches@gcc.gnu.org, Kito Cheng , Jim Wilson , Palmer Dabbelt , Andrew Waterman , Philipp Tomsich References: <20231020095348.2455729-1-christoph.muellner@vrull.eu> <20231020095348.2455729-2-christoph.muellner@vrull.eu> From: Jeff Law In-Reply-To: <20231020095348.2455729-2-christoph.muellner@vrull.eu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_MANYTO,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 10/20/23 03:53, Christoph Muellner wrote: > From: Christoph Müllner > > The XTheadMemIdx ISA extension provides a additional load and store > instructions with new addressing modes. > > The following memory accesses types are supported: > * load: b,bu,h,hu,w,wu,d > * store: b,h,w,d > > The following addressing modes are supported: > * immediate offset with PRE_MODIFY or POST_MODIFY (22 instructions): > l.ia, l.ib, s.ia, s.ib > * register offset with additional immediate offset (11 instructions): > lr, sr > * zero-extended register offset with additional immediate offset > (11 instructions): lur, sur > > The RISC-V base ISA does not support index registers, so the changes > are kept separate from the RISC-V standard support as much as possible. > > To combine the shift/multiply instructions into the memory access > instructions, this patch comes with a few insn_and_split optimizations > that allow the combiner to do this task. > > Handling the different cases of extensions results in a couple of INSNs > that look redundant on first view, but they are just the equivalence > of what we already have for Zbb as well. The only difference is, that > we have much more load instructions. > > We already have a constraint with the name 'th_f_fmv', therefore, > the new constraints follow this pattern and have the same length > as required ('th_m_mia', 'th_m_mib', 'th_m_mir', 'th_m_miu'). > > The added tests ensure that this feature won't regress without notice. > Testing: GCC regression test suite, GCC bootstrap build, and > SPEC CPU 2017 intrate (base&peak) on C920. > > Signed-off-by: Christoph Müllner > > gcc/ChangeLog: > > * config/riscv/constraints.md (th_m_mia): New constraint. > (th_m_mib): Likewise. > (th_m_mir): Likewise. > (th_m_miu): Likewise. > * config/riscv/riscv-protos.h (enum riscv_address_type): > Add new address types ADDRESS_REG_REG, ADDRESS_REG_UREG, > and ADDRESS_REG_WB and their documentation. > (struct riscv_address_info): Add new field 'shift' and > document the field usage for the new address types. > (riscv_valid_base_register_p): New prototype. > (th_memidx_legitimate_modify_p): Likewise. > (th_memidx_legitimate_index_p): Likewise. > (th_classify_address): Likewise. > (th_output_move): Likewise. > (th_print_operand_address): Likewise. > * config/riscv/riscv.cc (riscv_index_reg_class): > Return GR_REGS for XTheadMemIdx. > (riscv_regno_ok_for_index_p): Add support for XTheadMemIdx. > (riscv_classify_address): Call th_classify_address() on top. > (riscv_output_move): Call th_output_move() on top. > (riscv_print_operand_address): Call th_print_operand_address() > on top. > * config/riscv/riscv.h (HAVE_POST_MODIFY_DISP): New macro. > (HAVE_PRE_MODIFY_DISP): Likewise. > * config/riscv/riscv.md (zero_extendqi2): Disable > for XTheadMemIdx. > (*zero_extendqi2_internal): Convert to expand, > create INSN with same name and disable it for XTheadMemIdx. > (extendsidi2): Likewise. > (*extendsidi2_internal): Disable for XTheadMemIdx. > * config/riscv/thead.cc (valid_signed_immediate): New helper > function. > (th_memidx_classify_address_modify): New function. > (th_memidx_legitimate_modify_p): Likewise. > (th_memidx_output_modify): Likewise. > (is_memidx_mode): Likewise. > (th_memidx_classify_address_index): Likewise. > (th_memidx_legitimate_index_p): Likewise. > (th_memidx_output_index): Likewise. > (th_classify_address): Likewise. > (th_output_move): Likewise. > (th_print_operand_address): Likewise. > * config/riscv/thead.md (*th_memidx_operand): New splitter. > (*th_memidx_zero_extendqi2): New INSN. > (*th_memidx_extendsidi2): Likewise. > (*th_memidx_zero_extendsidi2): Likewise. > (*th_memidx_zero_extendhi2): Likewise. > (*th_memidx_extend2): Likewise. > (*th_memidx_bb_zero_extendsidi2): Likewise. > (*th_memidx_bb_zero_extendhi2): Likewise. > (*th_memidx_bb_extendhi2): Likewise. > (*th_memidx_bb_extendqi2): Likewise. > (TH_M_ANYI): New mode iterator. > (TH_M_NOEXTI): Likewise. > (*th_memidx_I_a): New combiner optimization. > (*th_memidx_I_b): Likewise. > (*th_memidx_I_c): Likewise. > (*th_memidx_US_a): Likewise. > (*th_memidx_US_b): Likewise. > (*th_memidx_US_c): Likewise. > (*th_memidx_UZ_a): Likewise. > (*th_memidx_UZ_b): Likewise. > (*th_memidx_UZ_c): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/xtheadmemidx-helpers.h: New test. > * gcc.target/riscv/xtheadmemidx-index-update.c: New test. > * gcc.target/riscv/xtheadmemidx-index-xtheadbb-update.c: New test. > * gcc.target/riscv/xtheadmemidx-index-xtheadbb.c: New test. > * gcc.target/riscv/xtheadmemidx-index.c: New test. > * gcc.target/riscv/xtheadmemidx-modify-xtheadbb.c: New test. > * gcc.target/riscv/xtheadmemidx-modify.c: New test. > * gcc.target/riscv/xtheadmemidx-uindex-update.c: New test. > * gcc.target/riscv/xtheadmemidx-uindex-xtheadbb-update.c: New test. > * gcc.target/riscv/xtheadmemidx-uindex-xtheadbb.c: New test. > * gcc.target/riscv/xtheadmemidx-uindex.c: New test. > --- > ); > @@ -5581,6 +5594,9 @@ riscv_print_operand_address (FILE *file, machine_mode mode ATTRIBUTE_UNUSED, rtx > { > struct riscv_address_info addr; > > + if (th_print_operand_address (file, mode, x)) > + return; > + > if (riscv_classify_address (&addr, x, word_mode, true)) > switch (addr.type) > { Check indentation in this ^^^ hunk, specifically the initial indentation of the IF and its THEN arm. > +} > + > +/* Provide a buffer for a th.lXia/th.lXib/th.sXia/th.sXib instruction > + for the given MODE. If LOAD is true, a load instruction will be > + provided (otherwise, a store instruction). If X is not suitable > + return NULL. */ > + > +static const char * > +th_memidx_output_modify (rtx x, machine_mode mode, bool load) > +{ > + static char buf[128] = {0}; A bit icky, though we don't have a threaded compiler so it's not catastrophic. Consider having the caller pass in a buffer rather than having it be static in the callee. It looks like you might need to do that 2 callers up, passing it through the intermediate functions. > + > +static const char * > +th_memidx_output_index (rtx x, machine_mode mode, bool load) > +{ > + struct riscv_address_info info; > + static char buf[128] = {0}; Similarly here. > @@ -387,4 +387,429 @@ (define_insn "*th_mempair_load_zero_extendsidi2" > (set_attr "mode" "DI") > (set_attr "length" "8")]) > > +;; XTheadMemIdx > + > +;; Help reload to add a displacement for the base register. > +;; In the case `zext(*(uN*)(base+(zext(rN)<<1)))` LRA splits > +;; off two new instructions: a) `new_base = base + disp`, and > +;; b) `index = zext(rN)<<1`. The index calculation has no > +;; corresponding instruction pattern and needs this insn_and_split > +;; to recover. > + > +(define_insn_and_split "*th_memidx_operand" > + [(set (match_operand:DI 0 "register_operand" "=r") > + (ashift:DI > + (zero_extend:DI (subreg:SI (match_operand:DI 1 "register_operand" "r") 0)) > + (match_operand 2 "const_int_operand" "n")))] > + "TARGET_64BIT && TARGET_XTHEADMEMIDX" > + "#" > + "" > + [(set (match_dup 0) (zero_extend:DI (subreg:SI (match_dup 1) 0))) > + (set (match_dup 0) (ashift:DI (match_dup 0) (match_dup 2)))] > + "" > + [(set_attr "type" "bitmanip")]) > + Interesting. I'd be curious if this triggers outside the reload context and if it's useful to allow that. Obviously not critical for this patch, but more of a curiosity. > +;; > +;; Note, that SHIFTs will be converted to MULTs during combine. More correctly, it's a canonicalization. It really doesn't have much to do with combine. From the canonicalization section in the manual: > @item > Within address computations (i.e., inside @code{mem}), a left shift is > converted into the appropriate multiplication by a power of two. > + > +/* If the shifted value is used later, we cannot eliminate it. */ > +/* { dg-final { scan-assembler-times "slli" 5 { target { rv32 } } } } */ > +/* { dg-final { scan-assembler-times "slli" 8 { target { rv64 } } } } */ Note we've started wrapping instructions in \M \m so that we don't get accidential matches with LTO output. It may not matter for these specific instances, but we might as well get into good habits. There should be numerous examples in the tree now. I think the only things that really need to be fixed to move forward are in the indention check and the wrapping of instructions in the scan-assembler directives. THe static buffer thing isn't critical -- whichever way you think the code is cleaner is fine with me. I don't think any of these changes necessitate waiting for an approval. Just post for archival purposes and commit. jeff