Re: [PATCH v3 4/6] RISC-V: Adds the prefix "th." for the instructions of XTheadVector.

[Jeff Law <jeffreyalaw@gmail.com>]

On 12/20/23 05:32, Jun Sha (Joshua) wrote:
> This patch adds th. prefix to all XTheadVector instructions by
> implementing new assembly output functions.
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv-protos.h
> 	(riscv_asm_output_opcode): New function.
> 	* config/riscv/riscv.cc (riscv_asm_output_opcode): Likewise.
> 	* config/riscv/riscv.h (ASM_OUTPUT_OPCODE): Likewise.
> 
> Co-authored-by: Jin Ma <jinma@linux.alibaba.com>
> Co-authored-by: Xianmiao Qu <cooper.qu@linux.alibaba.com>
> Co-authored-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>   gcc/config/riscv/riscv-protos.h               |  1 +
>   gcc/config/riscv/riscv.cc                     | 26 +++++++++++++++++++
>   gcc/config/riscv/riscv.h                      |  4 +++
>   .../riscv/rvv/xtheadvector/prefix.c           | 12 +++++++++
>   4 files changed, 43 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/prefix.c
> 
> diff --git a/gcc/config/riscv/riscv-protos.h b/gcc/config/riscv/riscv-protos.h
> index eaee53ce94e..f0eee71a18a 100644
> --- a/gcc/config/riscv/riscv-protos.h
> +++ b/gcc/config/riscv/riscv-protos.h
> @@ -101,6 +101,7 @@ struct riscv_address_info {
>   };
>   
>   /* Routines implemented in riscv.cc.  */
> +extern void riscv_asm_output_opcode(FILE *asm_out_file, const char *p);
>   extern enum riscv_symbol_type riscv_classify_symbolic_expression (rtx);
>   extern bool riscv_symbolic_constant_p (rtx, enum riscv_symbol_type *);
>   extern int riscv_float_const_rtx_index_for_fli (rtx);
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 8ae65760b6e..d3010bed8d8 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5595,6 +5595,32 @@ riscv_get_v_regno_alignment (machine_mode mode)
>     return lmul;
>   }
>   
> +void
> +riscv_asm_output_opcode(FILE *asm_out_file, const char *p)
Needs a function comment.  There's several examples in this file you can 
use to see the style we commonly use.  And a minor formatting nit, 
always put a space between a function name and an open paren.


> +{
> +  if (!TARGET_XTHEADVECTOR)
> +    return;
> +
> +  if (current_output_insn == NULL_RTX)
> +    return;
> +
> +  /* We need to handle the 'vset' special case here since it cannot
> +     be controlled by vector mode. */
> +  if (!strncmp (p, "vset", 4))
> +    {
> +      fputs ("th.", asm_out_file);
> +      return;
> +    }
> +
> +  subrtx_iterator::array_type array;
> +  FOR_EACH_SUBRTX (iter, array, PATTERN (current_output_insn), ALL)
> +    if (*iter && riscv_v_ext_mode_p (GET_MODE (*iter)) && p[0] == 'v')
> +      {
> +	fputs ("th.", asm_out_file);
> +	return;
> +      }
> +}
So rather than looking at the mode, would it make more sense to have an 
attribute (or re-use an existing attribute) to identify which opcodes 
are going to need prefixing?  We've got access to the INSN via 
current_output_insn.  So we can lookup attributes trivially.

This is a question, not a demand -- I'm looking for a solution that's 
going to be reliable with minimal effort going forward.

jeff