[Qemu-devel] [PATCH 0/2] target/riscv fixup and reserved argument checks

[Qemu-devel] [PATCH 0/2] target/riscv fixup and reserved argument checks

[Richard Henderson]

Based-on: https://github.com/palmer-dabbelt/qemu/tree/for-master

The first patch is my (interim) solution for compilation with clang.
I'm still open to Thomas' suggestion to add -Wno-typedef-redefinition
in the configure script.  I had intended to squash this into the
indicated patch when I got around to re-sending the patch set, but
Palmer has started queueing patches already.

The second patch handles the C.LWSP reserved operand case that Palmer
pointed out during review, and 4 others that I found while double
checking against the RVC instruction set listings.


r~


Richard Henderson (2):
  fixup! target/riscv: Name the argument sets for all of insn32 formats
  target/riscv: Add checks for several RVC reserved operands

 target/riscv/translate.c      | 13 ++++++++++---
 target/riscv/insn16-64.decode | 10 ++++++++--
 target/riscv/insn16.decode    |  7 ++++++-
 3 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] fixup! target/riscv: Name the argument sets for all of insn32 formats

[Richard Henderson]

---
 target/riscv/translate.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index d1f599a92d..009c146e8f 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -713,12 +713,19 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
  * initially declared by the 32-bit decoder, which results in duplicate
  * declaration warnings.  Suppress them.
  */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wredundant-decls"
+#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wredundant-decls"
+# ifdef __clang__
+#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"
+# endif
+#endif
 
 #include "decode_insn16.inc.c"
 
-#pragma GCC diagnostic pop
+#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+# pragma GCC diagnostic pop
+#endif
 
 static void decode_opc(DisasContext *ctx)
 {
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] target/riscv: Add checks for several RVC reserved operands

[Richard Henderson]

C.ADDI16SP, C.LWSP, C.JR, C.ADDIW, C.LDSP all have reserved
operands that were not diagnosed.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/insn16-64.decode | 10 ++++++++--
 target/riscv/insn16.decode    |  7 ++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/target/riscv/insn16-64.decode b/target/riscv/insn16-64.decode
index 055859d29f..672e1e916f 100644
--- a/target/riscv/insn16-64.decode
+++ b/target/riscv/insn16-64.decode
@@ -21,10 +21,16 @@ ld                011  ... ... .. ... 00 @cl_d
 sd                111  ... ... .. ... 00 @cs_d
 
 # *** RV64C Standard Extension (Quadrant 1) ***
-addiw             001 .  .....  ..... 01 @ci
+{
+  illegal         001 -  00000  ----- 01 # c.addiw, RES rd=0
+  addiw           001 .  .....  ..... 01 @ci
+}
 subw              100 1 11 ... 00 ... 01 @cs_2
 addw              100 1 11 ... 01 ... 01 @cs_2
 
 # *** RV64C Standard Extension (Quadrant 2) ***
-ld                011 .  .....  ..... 10 @c_ldsp
+{
+  illegal         011 -  00000  ----- 10 # c.ldsp, RES rd=0
+  ld              011 .  .....  ..... 10 @c_ldsp
+}
 sd                111 .  .....  ..... 10 @c_sdsp
diff --git a/target/riscv/insn16.decode b/target/riscv/insn16.decode
index 433c0e8c68..c06073ee72 100644
--- a/target/riscv/insn16.decode
+++ b/target/riscv/insn16.decode
@@ -96,6 +96,7 @@ sw                110  ... ... .. ... 00 @cs_w
 addi              000 .  .....  ..... 01 @ci
 addi              010 .  .....  ..... 01 @c_li
 {
+  illegal         011 0  -----  00000 01 # c.addi16sp, RES nzimm=0
   addi            011 .  00010  ..... 01 @c_addi16sp
   lui             011 .  .....  ..... 01 @c_lui
 }
@@ -113,8 +114,12 @@ bne               111  ... ...  ..... 01 @cb_z
 # *** RV32/64C Standard Extension (Quadrant 2) ***
 slli              000 .  .....  ..... 10 @c_shift2
 fld               001 .  .....  ..... 10 @c_ldsp
-lw                010 .  .....  ..... 10 @c_lwsp
 {
+  illegal         010 -  00000  ----- 10 # c.lwsp, RES rd=0
+  lw              010 .  .....  ..... 10 @c_lwsp
+}
+{
+  illegal         100 0  00000  00000 10 # c.jr, RES rs1=0
   jalr            100 0  .....  00000 10 @c_jalr rd=0  # C.JR
   addi            100 0  .....  ..... 10 @c_mv
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Add checks for several RVC reserved operands

[Richard Henderson]

On 4/25/19 10:26 AM, Richard Henderson wrote:
>  {
> +  illegal         011 0  -----  00000 01 # c.addi16sp, RES nzimm=0
>    addi            011 .  00010  ..... 01 @c_addi16sp
>    lui             011 .  .....  ..... 01 @c_lui
>  }

Bah.  I just realized the comment should be more like

  # c.addi16sp and c.lui, RES nzimm=0

Otherwise one is led to believe that rd=2 is missing
from the illegal pattern.


r~

Re: [Qemu-devel] [PATCH 1/2] fixup! target/riscv: Name the argument sets for all of insn32 formats

[Alistair Francis]

On Thu, Apr 25, 2019 at 10:28 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> ---
>  target/riscv/translate.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Not sure how this will work with a fixup, but still:
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d1f599a92d..009c146e8f 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -713,12 +713,19 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>   * initially declared by the 32-bit decoder, which results in duplicate
>   * declaration warnings.  Suppress them.
>   */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wredundant-decls"
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Wredundant-decls"
> +# ifdef __clang__
> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"
> +# endif
> +#endif
>
>  #include "decode_insn16.inc.c"
>
> -#pragma GCC diagnostic pop
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic pop
> +#endif
>
>  static void decode_opc(DisasContext *ctx)
>  {
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH 1/2] fixup! target/riscv: Name the argument sets for all of insn32 formats

[Alistair Francis]

On Thu, Apr 25, 2019 at 10:28 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> ---
>  target/riscv/translate.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Not sure how this will work with a fixup, but still:
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d1f599a92d..009c146e8f 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -713,12 +713,19 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>   * initially declared by the 32-bit decoder, which results in duplicate
>   * declaration warnings.  Suppress them.
>   */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wredundant-decls"
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Wredundant-decls"
> +# ifdef __clang__
> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"
> +# endif
> +#endif
>
>  #include "decode_insn16.inc.c"
>
> -#pragma GCC diagnostic pop
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic pop
> +#endif
>
>  static void decode_opc(DisasContext *ctx)
>  {
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Add checks for several RVC reserved operands

[Palmer Dabbelt]

On Thu, 25 Apr 2019 10:32:43 PDT (-0700), richard.henderson@linaro.org wrote:
> On 4/25/19 10:26 AM, Richard Henderson wrote:
>>  {
>> +  illegal         011 0  -----  00000 01 # c.addi16sp, RES nzimm=0
>>    addi            011 .  00010  ..... 01 @c_addi16sp
>>    lui             011 .  .....  ..... 01 @c_lui
>>  }
>
> Bah.  I just realized the comment should be more like
>
>   # c.addi16sp and c.lui, RES nzimm=0
>
> Otherwise one is led to believe that rd=2 is missing
> from the illegal pattern.

OK, I went ahead and squashed in that fix as well.

Re: [Qemu-devel] [PATCH 1/2] fixup! target/riscv: Name the argument sets for all of insn32 formats

[Palmer Dabbelt]

On Thu, 25 Apr 2019 10:26:35 PDT (-0700), richard.henderson@linaro.org wrote:
> ---
>  target/riscv/translate.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index d1f599a92d..009c146e8f 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -713,12 +713,19 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>   * initially declared by the 32-bit decoder, which results in duplicate
>   * declaration warnings.  Suppress them.
>   */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wredundant-decls"
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Wredundant-decls"
> +# ifdef __clang__
> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"
> +# endif
> +#endif
>
>  #include "decode_insn16.inc.c"
>
> -#pragma GCC diagnostic pop
> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
> +# pragma GCC diagnostic pop
> +#endif
>
>  static void decode_opc(DisasContext *ctx)
>  {

I've attemeted to fixup the patch on my for-master, let me know if I've screwed
something up!