[PATCH] target/i386: fix INSERTQ implementation

[PATCH] target/i386: fix INSERTQ implementation

[Paolo Bonzini]

INSERTQ is defined to not modify any bits in the lower 64 bits of the
destination, other than the ones being replaced with bits from the
source operand.  QEMU instead is using unshifted bits from the source
for those bits.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/ops_sse.h        | 10 +++++-----
 target/i386/ops_sse_header.h |  2 +-
 target/i386/tcg/translate.c  | 14 ++++++++++++--
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3504bca36a..7bf8bb967d 100644
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -934,7 +934,7 @@ void helper_extrq_i(CPUX86State *env, ZMMReg *d, int index, int length)
     d->ZMM_Q(0) = helper_extrq(d->ZMM_Q(0), index, length);
 }
 
-static inline uint64_t helper_insertq(uint64_t src, int shift, int len)
+static inline uint64_t helper_insertq(uint64_t dest, uint64_t src, int shift, int len)
 {
     uint64_t mask;
 
@@ -943,17 +943,17 @@ static inline uint64_t helper_insertq(uint64_t src, int shift, int len)
     } else {
         mask = (1ULL << len) - 1;
     }
-    return (src & ~(mask << shift)) | ((src & mask) << shift);
+    return (dest & ~(mask << shift)) | ((src & mask) << shift);
 }
 
 void helper_insertq_r(CPUX86State *env, ZMMReg *d, ZMMReg *s)
 {
-    d->ZMM_Q(0) = helper_insertq(s->ZMM_Q(0), s->ZMM_B(9) & 63, s->ZMM_B(8) & 63);
+    d->ZMM_Q(0) = helper_insertq(d->ZMM_Q(0), s->ZMM_Q(0), s->ZMM_B(9) & 63, s->ZMM_B(8) & 63);
 }
 
-void helper_insertq_i(CPUX86State *env, ZMMReg *d, int index, int length)
+void helper_insertq_i(CPUX86State *env, ZMMReg *d, ZMMReg *s, int index, int length)
 {
-    d->ZMM_Q(0) = helper_insertq(d->ZMM_Q(0), index, length);
+    d->ZMM_Q(0) = helper_insertq(d->ZMM_Q(0), s->ZMM_Q(0), index, length);
 }
 #endif
 
diff --git a/target/i386/ops_sse_header.h b/target/i386/ops_sse_header.h
index d99464afb0..400b24c091 100644
--- a/target/i386/ops_sse_header.h
+++ b/target/i386/ops_sse_header.h
@@ -193,7 +193,7 @@ DEF_HELPER_3(rcpss, void, env, ZMMReg, ZMMReg)
 DEF_HELPER_3(extrq_r, void, env, ZMMReg, ZMMReg)
 DEF_HELPER_4(extrq_i, void, env, ZMMReg, int, int)
 DEF_HELPER_3(insertq_r, void, env, ZMMReg, ZMMReg)
-DEF_HELPER_4(insertq_i, void, env, ZMMReg, int, int)
+DEF_HELPER_5(insertq_i, void, env, ZMMReg, ZMMReg, int, int)
 DEF_HELPER_3(glue(haddps, SUFFIX), void, env, ZMMReg, ZMMReg)
 DEF_HELPER_3(glue(haddpd, SUFFIX), void, env, ZMMReg, ZMMReg)
 DEF_HELPER_3(glue(hsubps, SUFFIX), void, env, ZMMReg, ZMMReg)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 8ec91d17af..5f31a59fb8 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3506,10 +3506,20 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
                     gen_helper_extrq_i(cpu_env, s->ptr0,
                                        tcg_const_i32(bit_index),
                                        tcg_const_i32(field_length));
-                else
-                    gen_helper_insertq_i(cpu_env, s->ptr0,
+                else {
+                    if (mod != 3) {
+                        gen_lea_modrm(env, s, modrm);
+                        op2_offset = offsetof(CPUX86State, xmm_t0);
+                        gen_ldq_env_A0(s, offsetof(CPUX86State, xmm_t0.ZMM_D(0)));
+                    } else {
+                        rm = (modrm & 7) | REX_B(s);
+                        op2_offset = ZMM_OFFSET(rm);
+                    }
+                    tcg_gen_addi_ptr(s->ptr1, cpu_env, op2_offset);
+                    gen_helper_insertq_i(cpu_env, s->ptr0, s->ptr1,
                                          tcg_const_i32(bit_index),
                                          tcg_const_i32(field_length));
+                }
             }
             break;
         case 0x7e: /* movd ea, mm */
-- 
2.37.2

Re: [PATCH] target/i386: fix INSERTQ implementation

[Richard Henderson]

On 9/18/22 09:56, Paolo Bonzini wrote:
> +                else {
> +                    if (mod != 3) {
> +                        gen_lea_modrm(env, s, modrm);
> +                        op2_offset = offsetof(CPUX86State, xmm_t0);
> +                        gen_ldq_env_A0(s, offsetof(CPUX86State, xmm_t0.ZMM_D(0)));

INSERTQ doesn't support a memory source.  The two forms are

INSERTQ xmm1, xmm2, imm8, imm8
INSERTQ xmm1, xmm2


r~