Re: [PATCH 08/25] perf arm_spe: Consolidate operation types

[James Clark <james.clark@linaro.org>]

On 29/09/2025 5:37 pm, Leo Yan wrote:
> Consolidate operation types in a way:
> 
> (a) The second-level types for memory and SIMD operations are classified
>      by modules. E.g., an operation may relate to general register,
>      SIMD/FP, SVE, etc.
> 
> (b) The associated information tells details. E.g., an operation is
>      load or store, whether it is atomic operation, etc.
> 
> Start the enum items for the second-level types from 8 to accommodate
> more entries within a 32-bit integer.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   tools/perf/util/arm-spe-decoder/arm-spe-decoder.h | 40 +++++++++++------------
>   1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> index 1259cbadfdc8098019afcd4cf65e733475310392..8156aa04f82e59ce345fb44223d3d22ecbc149a7 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> @@ -37,28 +37,28 @@ enum arm_spe_op_type {
>   	ARM_SPE_OP_LDST		= 1 << 1,
>   	ARM_SPE_OP_BRANCH_ERET	= 1 << 2,
>   
> -	/* Second level operation type for OTHER */
> -	ARM_SPE_OP_SVE_OTHER		= 1 << 16,
> -
> -	/* Second level operation type for LDST */
> -	ARM_SPE_OP_LD			= 1 << 16,
> -	ARM_SPE_OP_ST			= 1 << 17,
> -	ARM_SPE_OP_ATOMIC		= 1 << 18,
> -	ARM_SPE_OP_EXCL			= 1 << 19,
> -	ARM_SPE_OP_AR			= 1 << 20,
> -	ARM_SPE_OP_SIMD_FP		= 1 << 21,
> -	ARM_SPE_OP_GP_REG		= 1 << 22,
> -	ARM_SPE_OP_UNSPEC_REG		= 1 << 23,
> -	ARM_SPE_OP_NV_SYSREG		= 1 << 24,
> -	ARM_SPE_OP_SVE_LDST		= 1 << 25,
> +	/* Second level operation type for memory / SIMD */
> +	ARM_SPE_OP_GP_REG		= 1 << 8,
> +	ARM_SPE_OP_UNSPEC_REG		= 1 << 9,
> +	ARM_SPE_OP_NV_SYSREG		= 1 << 10,
> +	ARM_SPE_OP_SIMD_FP		= 1 << 11,
> +	ARM_SPE_OP_SVE_OTHER		= 1 << 12,
> +	ARM_SPE_OP_SVE_LDST		= 1 << 13,
> +
> +	/* Assisted information for memory / SIMD */
> +	ARM_SPE_OP_LD			= 1 << 20,
> +	ARM_SPE_OP_ST			= 1 << 21,
> +	ARM_SPE_OP_ATOMIC		= 1 << 22,
> +	ARM_SPE_OP_EXCL			= 1 << 23,
> +	ARM_SPE_OP_AR			= 1 << 24,
>   
>   	/* Second level operation type for BRANCH_ERET */
> -	ARM_SPE_OP_BR_COND		= 1 << 16,
> -	ARM_SPE_OP_BR_INDIRECT		= 1 << 17,
> -	ARM_SPE_OP_BR_GCS		= 1 << 18,
> -	ARM_SPE_OP_BR_CR_BL		= 1 << 19,
> -	ARM_SPE_OP_BR_CR_RET		= 1 << 20,
> -	ARM_SPE_OP_BR_CR_NON_BL_RET	= 1 << 21,
> +	ARM_SPE_OP_BR_COND		= 1 << 8,

I know it was already like this, but this should be multiple enums 
stored in a union. Having an enum with duplicate values is a bit of an 
abuse of the language. It takes more effort to understand it to 
carefully make future modifications too.

With multiple enums you don't need to rely on a comment to describe 
them, because that info would be in the name, like "enum 
arm_spe_2nd_op_ldst", "enum arm_spe_2nd_op_mem" etc.


> +	ARM_SPE_OP_BR_INDIRECT		= 1 << 9,
> +	ARM_SPE_OP_BR_GCS		= 1 << 10,
> +	ARM_SPE_OP_BR_CR_BL		= 1 << 11,
> +	ARM_SPE_OP_BR_CR_RET		= 1 << 12,
> +	ARM_SPE_OP_BR_CR_NON_BL_RET	= 1 << 13,
>   };
>   
>   enum arm_spe_common_data_source {
>