From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52b.google.com (mail-pg1-x52b.google.com [IPv6:2607:f8b0:4864:20::52b]) by sourceware.org (Postfix) with ESMTPS id 2D7573858D1E for ; Tue, 31 Oct 2023 13:43:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2D7573858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=vrull.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=vrull.eu ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2D7573858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::52b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698759788; cv=none; b=wb3+0LjEDNhOr7x4KOJuAUaNHgbcjP1mquw7twnkbiAjnj06FsMclQDVD5DujRf3uJlttWns4lbDX4/fFvErVu8Vg+Mh2juX/xpkr253HWxRWEbzXahUDhf4O/BlRxfOPIZc3jtMMpaGuaFTT5So0D/2ZgkPDjpVuWHi/BFasyM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698759788; c=relaxed/simple; bh=A7zm+s0wtn79HEC2GKmkQ6IQsao6Meoj/6c77DbTxdo=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=bFuEKf6a5MV2RUPAVrzLE2TS3X6KE/8PPAKQVyA5quHdjKn3KQhzYXcUj373WlGQs0shoguScEkleEFLXFhTOplKvpFl/2a4wtVkn2Ad0hCX/RwF3bILX1B/NjEwlmhmnA5T/yM1vxKsObVkdMpigtl3bJ+8VBI11bMLdLcSKJ4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pg1-x52b.google.com with SMTP id 41be03b00d2f7-5b856d73a12so4494435a12.1 for ; Tue, 31 Oct 2023 06:43:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull.eu; s=google; t=1698759785; x=1699364585; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=BZgseThCeT/ZsZgEs1cPSkrrc4ekARX47Xm/T7AK/WY=; b=jEXM6OyaFJTdS1EWfWBOtEP+Ga5vzsuMWw/mJHznvVZYYYuXeMWMT/0a+d4sgWMjf2 4wDuiOdhefEN0rwrMKp40STz2XdgKWX1k0waRUiVkN5itg//EBjw4KF8nAFh+iRJ6e1Z rPpMcqSHy+V4NDKmK05ybmcf1aQyfhpcKtxuJc4uKimIAEH79IIaQh/E+g5W4OtUHpWS 9e1zvsD+cfNYq4m0kNE0BqLkS4ABAjX/UE/iMYVePUmra5vpr6yUP+rYtiumgVK6RJ1v OMKLOvIFQZOygHwARJg7hlWCT4x1MFECBCD1eBzTcArBpvVHz8wbPs/HD1U04QdXVWXG BFiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698759785; x=1699364585; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BZgseThCeT/ZsZgEs1cPSkrrc4ekARX47Xm/T7AK/WY=; b=Gdp1e+L96jawbxqpqulJW2zp6WX/PCJMTujNbsJqHU0yVub1p1mykr5cySVUE/R94z vL+UalqdLOYUeR4tWcJ0z1CAgHbxPeLLEbehnFe7E/1B9bLK9z1lpGdlD0eFwHbowDgQ Ah8COnrU4m+lXIcn01nqGQLPWQGv3qfTqN1jlL8qP9O7K8O6gGprG1Xx/2ZURGVcBWLD FUxkbXxLOmeOtLSdaTN3YODuMZg14z6/h1P4z+4SR3cO5JVTzNnnZ8pnHjZQrwzGT6BG RgwFWJenE8HS1O0URgvd3wgxSBHqhnj22kwak2GBpQJ9t0WwHvM8JuCshPh5qS8Wgybr 6/iQ== X-Gm-Message-State: AOJu0YyG+wesQ9AAJNZk81lNNruesNgSDRONMoMtq2DOJVwGkvXQU7B0 mK9REbQniAaLzH5pyg6eypkkMSNKyEuzQAcOIZY6hQ== X-Google-Smtp-Source: AGHT+IGVV3E54/a6mRW0IruMXBBtYKgBOPCohIL7ayqA5sOgWcrcwDGXyAZ68KtEMSuNyoYJ9OfffP9NQXH2w0+0vHc= X-Received: by 2002:a17:90b:46c8:b0:27d:8ad:c4e1 with SMTP id jx8-20020a17090b46c800b0027d08adc4e1mr11143210pjb.19.1698759784912; Tue, 31 Oct 2023 06:43:04 -0700 (PDT) MIME-Version: 1.0 References: <20231020095348.2455729-1-christoph.muellner@vrull.eu> <20231020095348.2455729-2-christoph.muellner@vrull.eu> In-Reply-To: From: =?UTF-8?Q?Christoph_M=C3=BCllner?= Date: Tue, 31 Oct 2023 14:42:49 +0100 Message-ID: Subject: Re: [PATCH v2 1/2] riscv: thead: Add support for the XTheadMemIdx ISA extension To: Jeff Law Cc: gcc-patches@gcc.gnu.org, Kito Cheng , Jim Wilson , Palmer Dabbelt , Andrew Waterman , Philipp Tomsich Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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 Sun, Oct 29, 2023 at 10:44=E2=80=AFPM Jeff Law w= rote: > > > > On 10/20/23 03:53, Christoph Muellner wrote: > > From: Christoph M=C3=BCllner > > > > 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=C3=BCllner > > > > 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 tes= t. > > * 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. I think the indentation looks ok (and check_GNU_style did not complain). The structure of the function is as follows: static void f (...) { if (th_print_operand_address ()) return; if (foo (bar)) switch { case A: baz (); return; default: gcc_unreachable (); } gcc_unreachable () } So the then-arm just consists of a switch statement. Would you prefer braces around the switch statement? Or, is there something that I fail to see? > > +} > > + > > +/* 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] =3D {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. In my defense, I am not the first to use this pattern (allocating a static char array, filling it with snprintf and returning a pointer to the array). Some examples from other backends: * aarch64.cc: aarch64_output_simd_mov_immediate * arm/vfp.md: push_fpsysreg_insn * i386/i386.cc: output_387_ffreep * rs6000/darwin.md: @reload_macho_picbase_ I think your main concern is about returning a static-allocated-array. If we would allocate the buffer in th_output_move, then we would still have to use "static", because also this function returns the string. Therefore, I will do the following: * drop the "static" (allocate on the stack) * call output_asm_insn() * return "" > > + > > +static const char * > > +th_memidx_output_index (rtx x, machine_mode mode, bool load) > > +{ > > + struct riscv_address_info info; > > + static char buf[128] =3D {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 =3D base + disp`, and > > +;; b) `index =3D 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" "=3Dr") > > + (ashift:DI > > + (zero_extend:DI (subreg:SI (match_operand:DI 1 "register_operan= d" "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. I'm quite certain that this issue is specific to reload/LRA. At least I'm not aware of other passes that change existing insns such that they need to be split. As such I will gate this with lra_in_progress. > > +;; > > +;; 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. Will do. > 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. Thanks! > > jeff