[Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64

Stefan Weil posted 1 patch 5 years ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190410194838.10123-1-sw@weilnetz.de
Maintainers: Stefan Weil <sw@weilnetz.de>, Richard Henderson <rth@twiddle.net>
tcg/tci.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64
Posted by Stefan Weil 5 years ago
This fixes "make check-tcg" on a Debian x86_64 host.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 tcg/tci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tcg/tci.c b/tcg/tci.c
index 33edca1903..a6208653e8 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -127,6 +127,12 @@ static void tci_write_reg8(tcg_target_ulong *regs, TCGReg index, uint8_t value)
     tci_write_reg(regs, index, value);
 }
 
+static void
+tci_write_reg16(tcg_target_ulong *regs, TCGReg index, uint16_t value)
+{
+    tci_write_reg(regs, index, value);
+}
+
 static void
 tci_write_reg32(tcg_target_ulong *regs, TCGReg index, uint32_t value)
 {
@@ -585,6 +591,8 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2));
             break;
         case INDEX_op_ld8s_i32:
+            TODO();
+            break;
         case INDEX_op_ld16u_i32:
             TODO();
             break;
@@ -854,7 +862,14 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2));
             break;
         case INDEX_op_ld8s_i64:
+            TODO();
+            break;
         case INDEX_op_ld16u_i64:
+            t0 = *tb_ptr++;
+            t1 = tci_read_r(regs, &tb_ptr);
+            t2 = tci_read_s32(&tb_ptr);
+            tci_write_reg16(regs, t0, *(uint16_t *)(t1 + t2));
+            break;
         case INDEX_op_ld16s_i64:
             TODO();
             break;
-- 
2.20.1


Re: [Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64
Posted by Richard Henderson 5 years ago
On 4/10/19 9:48 AM, Stefan Weil wrote:
> @@ -127,6 +127,12 @@ static void tci_write_reg8(tcg_target_ulong *regs, TCGReg index, uint8_t value)
>      tci_write_reg(regs, index, value);
>  }
>  
> +static void
> +tci_write_reg16(tcg_target_ulong *regs, TCGReg index, uint16_t value)
> +{
> +    tci_write_reg(regs, index, value);
> +}
> +
>  static void
>  tci_write_reg32(tcg_target_ulong *regs, TCGReg index, uint32_t value)
>  {
> @@ -854,7 +862,14 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2));
>              break;
>          case INDEX_op_ld8s_i64:
> +            TODO();
> +            break;
>          case INDEX_op_ld16u_i64:
> +            t0 = *tb_ptr++;
> +            t1 = tci_read_r(regs, &tb_ptr);
> +            t2 = tci_read_s32(&tb_ptr);
> +            tci_write_reg16(regs, t0, *(uint16_t *)(t1 + t2));
> +            break;

Looks ok, I guess, although the introduction of tci_write_reg16 seems redundant
with the uint16_t value that is loaded.

Why not use tci_write_reg64, since that is the size of the register you're
modifying?  Let the zero-extension explicit in the name of the opcode to be
reflected in the zero-extension implied by the passing of a uint16_t value to a
uint64_t argument.


r~

Re: [Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64
Posted by Stefan Weil 5 years ago
On 11.04.19 08:46, Richard Henderson wrote:
> Looks ok, I guess, although the introduction of tci_write_reg16 seems redundant
> with the uint16_t value that is loaded.

It could directly call tci_write_reg, but the new code is similar to the
existing code which also uses the same kind of indirection.

The resulting binary code should be the same, as all those small
functions can be inlined by an optimizing compiler.

> Why not use tci_write_reg64, since that is the size of the register you're
> modifying?  Let the zero-extension explicit in the name of the opcode to be
> reflected in the zero-extension implied by the passing of a uint16_t value to a
> uint64_t argument.

I think that using tci_write_reg64 would be wrong on 32 bit hosts.

Regards
Stefan




Re: [Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64
Posted by Richard Henderson 5 years ago
On 4/10/19 9:37 PM, Stefan Weil wrote:
>> Why not use tci_write_reg64, since that is the size of the register you're
>> modifying?  Let the zero-extension explicit in the name of the opcode to be
>> reflected in the zero-extension implied by the passing of a uint16_t value to a
>> uint64_t argument.
> 
> I think that using tci_write_reg64 would be wrong on 32 bit hosts.

... as would using INDEX_op_ld16u_i64 in the first place.
This is already within #if TCG_TARGET_REG_BITS == 64.


r~

Re: [Qemu-devel] [PATCH] tci: Add implementation for INDEX_op_ld16u_i64
Posted by Thomas Huth 5 years ago
On 10/04/2019 21.48, Stefan Weil wrote:
> This fixes "make check-tcg" on a Debian x86_64 host.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  tcg/tci.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tcg/tci.c b/tcg/tci.c
> index 33edca1903..a6208653e8 100644
> --- a/tcg/tci.c
> +++ b/tcg/tci.c
> @@ -127,6 +127,12 @@ static void tci_write_reg8(tcg_target_ulong *regs, TCGReg index, uint8_t value)
>      tci_write_reg(regs, index, value);
>  }
>  
> +static void
> +tci_write_reg16(tcg_target_ulong *regs, TCGReg index, uint16_t value)
> +{
> +    tci_write_reg(regs, index, value);
> +}
> +
>  static void
>  tci_write_reg32(tcg_target_ulong *regs, TCGReg index, uint32_t value)
>  {
> @@ -585,6 +591,8 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2));
>              break;
>          case INDEX_op_ld8s_i32:
> +            TODO();
> +            break;
>          case INDEX_op_ld16u_i32:
>              TODO();
>              break;
> @@ -854,7 +862,14 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2));
>              break;
>          case INDEX_op_ld8s_i64:
> +            TODO();
> +            break;
>          case INDEX_op_ld16u_i64:
> +            t0 = *tb_ptr++;
> +            t1 = tci_read_r(regs, &tb_ptr);
> +            t2 = tci_read_s32(&tb_ptr);
> +            tci_write_reg16(regs, t0, *(uint16_t *)(t1 + t2));
> +            break;
>          case INDEX_op_ld16s_i64:
>              TODO();
>              break;
> 

Works for me, too:

 https://gitlab.com/huth/qemu/-/jobs/194622472

Tested-by: Thomas Huth <thuth@redhat.com>