Re: [PATCH v3 04/21] target/riscv: additional macros to check instruction support

[Richard Henderson]

On 10/19/21 2:47 AM, Frédéric Pétrot wrote:
> Given that the 128-bit version of the riscv spec adds new instructions, and
> that some instructions that were previously only available in 64-bit mode
> are now available for both 64-bit and 128-bit, we added new macros to check
> for the processor mode during translation.
>
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> Co-authored-by: Fabien Portas <fabien.portas@grenoble-inp.org>
> ---
>   target/riscv/translate.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 35245aafa7..121fcd71fe 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -350,6 +350,24 @@ EX_SH(12)
>       }                              \
>   } while (0)
>   
> +#define REQUIRE_128BIT(ctx) do {   \
> +    if (get_xl(ctx) < MXL_RV128) { \
> +        return false;              \
> +    }                              \
> +} while (0)
> +
> +#define REQUIRE_32_OR_64BIT(ctx) do { \
> +    if (get_xl(ctx) == MXL_RV128) {   \
> +        return false;                 \
> +    }                                 \
> +} while (0)
> +
> +#define REQUIRE_64_OR_128BIT(ctx) do { \
> +    if (get_xl(ctx) == MXL_RV32) {     \
> +        return false;                  \
> +    }                                  \
> +} while (0)

So... you've left REQUIRE_64BIT accepting RV128, so that means that your current 
REQUIRE_64_OR_128BIT is redundant.  Is that intentional?

It does seem like all places that accept RV128 should accept RV64, but perhaps that's just 
your "limited" caveat in the cover letter.

You don't use REQUIRE_32_OR_64BIT at all.  Remove it?


r~