Re: [PATCH v2 1/2] riscv: thead: Add support for the XTheadMemIdx ISA extension

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/20/23 03:53, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
> 
> 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<ltype>.ia, l<ltype>.ib, s<stype>.ia, s<stype>.ib
> * register offset with additional immediate offset (11 instructions):
>    lr<ltype>, sr<stype>
> * zero-extended register offset with additional immediate offset
>    (11 instructions): lur<ltype>, sur<stype>
> 
> 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 <christoph.muellner@vrull.eu>
> 
> 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_extendqi<SUPERQI:mode>2): Disable
> 	for XTheadMemIdx.
> 	(*zero_extendqi<SUPERQI:mode>2_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_extendqi<SUPERQI:mode>2): New INSN.
> 	(*th_memidx_extendsidi2): Likewise.
> 	(*th_memidx_zero_extendsidi2): Likewise.
> 	(*th_memidx_zero_extendhi<GPR:mode>2): Likewise.
> 	(*th_memidx_extend<SHORT:mode><SUPERQI:mode>2): Likewise.
> 	(*th_memidx_bb_zero_extendsidi2): Likewise.
> 	(*th_memidx_bb_zero_extendhi<GPR:mode>2): Likewise.
> 	(*th_memidx_bb_extendhi<GPR:mode>2): Likewise.
> 	(*th_memidx_bb_extendqi<SUPERQI:mode>2): 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