[v3] Adding partial support for 128-bit riscv target

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

Posted by Frédéric Pétrot 4 years, 3 months ago

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)
+
 static int ex_rvc_register(DisasContext *ctx, int reg)
 {
     return 8 + reg;
-- 
2.33.0

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

Posted by Richard Henderson 4 years, 3 months ago

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~

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

Posted by Frédéric Pétrot 4 years, 3 months ago

Le 20/10/2021 à 16:08, Richard Henderson a écrit :
> On 10/19/21 2:47 AM, Frédéric Pétrot wrote:
>> +
>> +#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.

  My bad, indeed there is no instruction only required by RV64. "Limited" was
  related to the minimal support of the priviledge spec.
> You don't use REQUIRE_32_OR_64BIT at all.  Remove it?

  It's a bug : some compressed insns are only RV32/RV64 (this is linked to
  the other bug in the order in which the insns stand in the insn16.decode
  file that you pointed out).

  Frédéric
> 
> 
> r~

-- 
+---------------------------------------------------------------------------+
| Frédéric Pétrot, Pr. Grenoble INP-Ensimag/TIMA,   Ensimag deputy director |
| Mob/Pho: +33 6 74 57 99 65/+33 4 76 57 48 70      Ad augusta  per angusta |
| http://tima.univ-grenoble-alpes.fr frederic.petrot@univ-grenoble-alpes.fr |
+---------------------------------------------------------------------------+