[PATCH] target/i386: fix INSERTQ implementation

Paolo Bonzini posted 1 patch 3 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220918075609.549781-1-pbonzini@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
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(-)
[PATCH] target/i386: fix INSERTQ implementation
Posted by Paolo Bonzini 3 years, 4 months ago
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
Posted by Richard Henderson 3 years, 4 months ago
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~