diff mbox series

[3/9] target/Hexagon: Finish conversion to tcg_gen_qemu_{ld, st}_*

Message ID 20230502135741.1158035-4-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Remove compatability helpers for qemu ld/st | expand

Commit Message

Richard Henderson May 2, 2023, 1:57 p.m. UTC
Convert away from the old interface with the implicit
MemOp argument.  Importantly, this removes some incorrect
casts generated by idef-parser's gen_load().

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/hexagon/macros.h                     | 14 ++++-----
 target/hexagon/genptr.c                     |  8 +++---
 target/hexagon/idef-parser/parser-helpers.c | 28 +++++++++---------
 target/hexagon/translate.c                  | 32 ++++++++++-----------
 4 files changed, 40 insertions(+), 42 deletions(-)

Comments

Taylor Simpson May 2, 2023, 3:27 p.m. UTC | #1
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, May 2, 2023 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: mrolnik@gmail.com; edgar.iglesias@gmail.com; Taylor Simpson
> <tsimpson@quicinc.com>; ale@rev.ng; anjo@rev.ng; laurent@vivier.eu;
> philmd@linaro.org; jiaxun.yang@flygoat.com; david@redhat.com;
> iii@linux.ibm.com; thuth@redhat.com; mark.cave-ayland@ilande.co.uk;
> atar4qemu@gmail.com; jcmvbkbc@gmail.com
> Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
> tcg_gen_qemu_{ld,st}_*
> 
> Convert away from the old interface with the implicit MemOp argument.
> Importantly, this removes some incorrect casts generated by idef-parser's
> gen_load().
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/hexagon/macros.h                     | 14 ++++-----
>  target/hexagon/genptr.c                     |  8 +++---
>  target/hexagon/idef-parser/parser-helpers.c | 28 +++++++++---------
>  target/hexagon/translate.c                  | 32 ++++++++++-----------
>  4 files changed, 40 insertions(+), 42 deletions(-)
> 
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 502c85ae35..244063b1d2 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv
> src)
> 
>  static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src) @@ -
> 1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx, intptr_t
> dstoff, TCGv src,
>          tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
>      }
>      for (int i = 0; i < sizeof(MMVector) / 8; i++) {
> -        tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
> +        tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
>          tcg_gen_addi_tl(src, src, 8);
>          tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);

Did you intend to leave the tcg_gen_st_i64 alone or should that be converted to tcg_gen_qemu_st64.

There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be converted?

>      }

I'm curious if there's a better way to do a vector load (e.g., with tcg_gen_gvec_<something>) than a loop that does 8 bytes at a time.

Otherwise,
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
Richard Henderson May 2, 2023, 3:48 p.m. UTC | #2
On 5/2/23 16:27, Taylor Simpson wrote:
> 
> 
>> -----Original Message-----
>> From: Richard Henderson <richard.henderson@linaro.org>
>> Sent: Tuesday, May 2, 2023 8:58 AM
>> To: qemu-devel@nongnu.org
>> Cc: mrolnik@gmail.com; edgar.iglesias@gmail.com; Taylor Simpson
>> <tsimpson@quicinc.com>; ale@rev.ng; anjo@rev.ng; laurent@vivier.eu;
>> philmd@linaro.org; jiaxun.yang@flygoat.com; david@redhat.com;
>> iii@linux.ibm.com; thuth@redhat.com; mark.cave-ayland@ilande.co.uk;
>> atar4qemu@gmail.com; jcmvbkbc@gmail.com
>> Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
>> tcg_gen_qemu_{ld,st}_*
>>
>> Convert away from the old interface with the implicit MemOp argument.
>> Importantly, this removes some incorrect casts generated by idef-parser's
>> gen_load().
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/hexagon/macros.h                     | 14 ++++-----
>>   target/hexagon/genptr.c                     |  8 +++---
>>   target/hexagon/idef-parser/parser-helpers.c | 28 +++++++++---------
>>   target/hexagon/translate.c                  | 32 ++++++++++-----------
>>   4 files changed, 40 insertions(+), 42 deletions(-)
>>
>> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
>> 502c85ae35..244063b1d2 100644
>> --- a/target/hexagon/genptr.c
>> +++ b/target/hexagon/genptr.c
>> @@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv
>> src)
>>
>>   static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src) @@ -
>> 1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx, intptr_t
>> dstoff, TCGv src,
>>           tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
>>       }
>>       for (int i = 0; i < sizeof(MMVector) / 8; i++) {
>> -        tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
>> +        tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
>>           tcg_gen_addi_tl(src, src, 8);
>>           tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);
> 
> Did you intend to leave the tcg_gen_st_i64 alone or should that be converted to tcg_gen_qemu_st64.
> 
> There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be converted?

No, those are host stores to env, not guest stores to guest memory.
Notice the lack of "qemu" in the function names.

> I'm curious if there's a better way to do a vector load (e.g., with tcg_gen_gvec_<something>) than a loop that does 8 bytes at a time.

The best you can do at the moment is tcg_gen_qemu_ld_i128.
But there's no gvec variant to load arbitrary vector lengths.


r~
Taylor Simpson May 2, 2023, 7:09 p.m. UTC | #3
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, May 2, 2023 10:49 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/9] target/Hexagon: Finish conversion to
> tcg_gen_qemu_{ld,st}_*
> 
> On 5/2/23 16:27, Taylor Simpson wrote:
> >
> >
> >> -----Original Message-----
> >> From: Richard Henderson <richard.henderson@linaro.org>
> >> Sent: Tuesday, May 2, 2023 8:58 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: mrolnik@gmail.com; edgar.iglesias@gmail.com; Taylor Simpson
> >> <tsimpson@quicinc.com>; ale@rev.ng; anjo@rev.ng; laurent@vivier.eu;
> >> philmd@linaro.org; jiaxun.yang@flygoat.com; david@redhat.com;
> >> iii@linux.ibm.com; thuth@redhat.com; mark.cave-ayland@ilande.co.uk;
> >> atar4qemu@gmail.com; jcmvbkbc@gmail.com
> >> Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
> >> tcg_gen_qemu_{ld,st}_*
> >>
> >> Convert away from the old interface with the implicit MemOp argument.
> >> Importantly, this removes some incorrect casts generated by
> >> idef-parser's gen_load().
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   target/hexagon/macros.h                     | 14 ++++-----
> >>   target/hexagon/genptr.c                     |  8 +++---
> >>   target/hexagon/idef-parser/parser-helpers.c | 28 +++++++++---------
> >>   target/hexagon/translate.c                  | 32 ++++++++++-----------
> >>   4 files changed, 40 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> >> 502c85ae35..244063b1d2 100644
> >> --- a/target/hexagon/genptr.c
> >> +++ b/target/hexagon/genptr.c
> >> @@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result,
> >> TCGv
> >> src)
> >>
> >>   static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src)
> >> @@ -
> >> 1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx,
> >> intptr_t dstoff, TCGv src,
> >>           tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
> >>       }
> >>       for (int i = 0; i < sizeof(MMVector) / 8; i++) {
> >> -        tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
> >> +        tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
> >>           tcg_gen_addi_tl(src, src, 8);
> >>           tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);
> >
> > Did you intend to leave the tcg_gen_st_i64 alone or should that be
> converted to tcg_gen_qemu_st64.
> >
> > There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be
> converted?
> 
> No, those are host stores to env, not guest stores to guest memory.
> Notice the lack of "qemu" in the function names.
> 
> > I'm curious if there's a better way to do a vector load (e.g., with
> tcg_gen_gvec_<something>) than a loop that does 8 bytes at a time.
> 
> The best you can do at the moment is tcg_gen_qemu_ld_i128.
> But there's no gvec variant to load arbitrary vector lengths.

OK, thanks.

Also,
Tested-by: Taylor Simpson <tsimpson@quicinc.com>
Xingtao Yao (Fujitsu)" via May 3, 2023, 12:47 p.m. UTC | #4
On 5/2/23 15:57, Richard Henderson wrote:
> Convert away from the old interface with the implicit
> MemOp argument.  Importantly, this removes some incorrect
> casts generated by idef-parser's gen_load().
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/hexagon/macros.h                     | 14 ++++-----
>   target/hexagon/genptr.c                     |  8 +++---
>   target/hexagon/idef-parser/parser-helpers.c | 28 +++++++++---------
>   target/hexagon/translate.c                  | 32 ++++++++++-----------
>   4 files changed, 40 insertions(+), 42 deletions(-)
Reviewed-by: Anton Johansson <anjo@rev.ng>
diff mbox series

Patch

diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 3e162de3a7..760630de8f 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -99,37 +99,37 @@ 
 #define MEM_LOAD1s(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 1); \
-        tcg_gen_qemu_ld8s(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_SB); \
     } while (0)
 #define MEM_LOAD1u(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 1); \
-        tcg_gen_qemu_ld8u(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_UB); \
     } while (0)
 #define MEM_LOAD2s(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 2); \
-        tcg_gen_qemu_ld16s(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_TESW); \
     } while (0)
 #define MEM_LOAD2u(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 2); \
-        tcg_gen_qemu_ld16u(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_TEUW); \
     } while (0)
 #define MEM_LOAD4s(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 4); \
-        tcg_gen_qemu_ld32s(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_TESL); \
     } while (0)
 #define MEM_LOAD4u(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 4); \
-        tcg_gen_qemu_ld32s(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_tl(DST, VA, ctx->mem_idx, MO_TEUL); \
     } while (0)
 #define MEM_LOAD8u(DST, VA) \
     do { \
         CHECK_NOSHUF(VA, 8); \
-        tcg_gen_qemu_ld64(DST, VA, ctx->mem_idx); \
+        tcg_gen_qemu_ld_i64(DST, VA, ctx->mem_idx, MO_TEUQ); \
     } while (0)
 
 #define MEM_STORE1_FUNC(X) \
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 502c85ae35..244063b1d2 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -320,14 +320,14 @@  void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
 
 static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
 {
-    tcg_gen_qemu_ld32u(dest, vaddr, mem_index);
+    tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
     tcg_gen_mov_tl(hex_llsc_addr, vaddr);
     tcg_gen_mov_tl(hex_llsc_val, dest);
 }
 
 static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int mem_index)
 {
-    tcg_gen_qemu_ld64(dest, vaddr, mem_index);
+    tcg_gen_qemu_ld_i64(dest, vaddr, mem_index, MO_TEUQ);
     tcg_gen_mov_tl(hex_llsc_addr, vaddr);
     tcg_gen_mov_i64(hex_llsc_val_i64, dest);
 }
@@ -678,7 +678,7 @@  static void gen_load_frame(DisasContext *ctx, TCGv_i64 frame, TCGv EA)
 {
     Insn *insn = ctx->insn;  /* Needed for CHECK_NOSHUF */
     CHECK_NOSHUF(EA, 8);
-    tcg_gen_qemu_ld64(frame, EA, ctx->mem_idx);
+    tcg_gen_qemu_ld_i64(frame, EA, ctx->mem_idx, MO_TEUQ);
 }
 
 static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src)
@@ -1019,7 +1019,7 @@  static void gen_vreg_load(DisasContext *ctx, intptr_t dstoff, TCGv src,
         tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
     }
     for (int i = 0; i < sizeof(MMVector) / 8; i++) {
-        tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
+        tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
         tcg_gen_addi_tl(src, src, 8);
         tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);
     }
diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c
index 86511efb62..8734218e51 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -1737,36 +1737,34 @@  void gen_load_cancel(Context *c, YYLTYPE *locp)
 void gen_load(Context *c, YYLTYPE *locp, HexValue *width,
               HexSignedness signedness, HexValue *ea, HexValue *dst)
 {
-    char size_suffix[4] = {0};
-    const char *sign_suffix;
+    unsigned dst_bit_width;
+    unsigned src_bit_width;
+
     /* Memop width is specified in the load macro */
     assert_signedness(c, locp, signedness);
-    sign_suffix = (width->imm.value > 4)
-                   ? ""
-                   : ((signedness == UNSIGNED) ? "u" : "s");
+
     /* If dst is a variable, assert that is declared and load the type info */
     if (dst->type == VARID) {
         find_variable(c, locp, dst, dst);
     }
 
-    snprintf(size_suffix, 4, "%" PRIu64, width->imm.value * 8);
+    src_bit_width = width->imm.value * 8;
+    dst_bit_width = MAX(dst->bit_width, 32);
+
     /* Lookup the effective address EA */
     find_variable(c, locp, ea, ea);
     OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_store_s1) {\n");
     OUT(c, locp, "probe_noshuf_load(", ea, ", ", width, ", ctx->mem_idx);\n");
     OUT(c, locp, "process_store(ctx, 1);\n");
     OUT(c, locp, "}\n");
-    OUT(c, locp, "tcg_gen_qemu_ld", size_suffix, sign_suffix);
+
+    OUT(c, locp, "tcg_gen_qemu_ld_i", &dst_bit_width);
     OUT(c, locp, "(");
-    if (dst->bit_width > width->imm.value * 8) {
-        /*
-         * Cast to the correct TCG type if necessary, to avoid implict cast
-         * warnings. This is needed when the width of the destination var is
-         * larger than the size of the requested load.
-         */
-        OUT(c, locp, "(TCGv) ");
+    OUT(c, locp, dst, ", ", ea, ", ctx->mem_idx, MO_", &src_bit_width);
+    if (signedness == SIGNED) {
+        OUT(c, locp, " | MO_SIGN");
     }
-    OUT(c, locp, dst, ", ", ea, ", ctx->mem_idx);\n");
+    OUT(c, locp, " | MO_TE);\n");
 }
 
 void gen_store(Context *c, YYLTYPE *locp, HexValue *width, HexValue *ea,
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index c087f183d0..cddd7c5db4 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -627,27 +627,27 @@  void process_store(DisasContext *ctx, int slot_num)
         switch (ctx->store_width[slot_num]) {
         case 1:
             gen_check_store_width(ctx, slot_num);
-            tcg_gen_qemu_st8(hex_store_val32[slot_num],
-                             hex_store_addr[slot_num],
-                             ctx->mem_idx);
+            tcg_gen_qemu_st_tl(hex_store_val32[slot_num],
+                               hex_store_addr[slot_num],
+                               ctx->mem_idx, MO_UB);
             break;
         case 2:
             gen_check_store_width(ctx, slot_num);
-            tcg_gen_qemu_st16(hex_store_val32[slot_num],
-                              hex_store_addr[slot_num],
-                              ctx->mem_idx);
+            tcg_gen_qemu_st_tl(hex_store_val32[slot_num],
+                               hex_store_addr[slot_num],
+                               ctx->mem_idx, MO_TEUW);
             break;
         case 4:
             gen_check_store_width(ctx, slot_num);
-            tcg_gen_qemu_st32(hex_store_val32[slot_num],
-                              hex_store_addr[slot_num],
-                              ctx->mem_idx);
+            tcg_gen_qemu_st_tl(hex_store_val32[slot_num],
+                               hex_store_addr[slot_num],
+                               ctx->mem_idx, MO_TEUL);
             break;
         case 8:
             gen_check_store_width(ctx, slot_num);
-            tcg_gen_qemu_st64(hex_store_val64[slot_num],
-                              hex_store_addr[slot_num],
-                              ctx->mem_idx);
+            tcg_gen_qemu_st_i64(hex_store_val64[slot_num],
+                                hex_store_addr[slot_num],
+                                ctx->mem_idx, MO_TEUQ);
             break;
         default:
             {
@@ -693,13 +693,13 @@  static void process_dczeroa(DisasContext *ctx)
         TCGv_i64 zero = tcg_constant_i64(0);
 
         tcg_gen_andi_tl(addr, hex_dczero_addr, ~0x1f);
-        tcg_gen_qemu_st64(zero, addr, ctx->mem_idx);
+        tcg_gen_qemu_st_i64(zero, addr, ctx->mem_idx, MO_UQ);
         tcg_gen_addi_tl(addr, addr, 8);
-        tcg_gen_qemu_st64(zero, addr, ctx->mem_idx);
+        tcg_gen_qemu_st_i64(zero, addr, ctx->mem_idx, MO_UQ);
         tcg_gen_addi_tl(addr, addr, 8);
-        tcg_gen_qemu_st64(zero, addr, ctx->mem_idx);
+        tcg_gen_qemu_st_i64(zero, addr, ctx->mem_idx, MO_UQ);
         tcg_gen_addi_tl(addr, addr, 8);
-        tcg_gen_qemu_st64(zero, addr, ctx->mem_idx);
+        tcg_gen_qemu_st_i64(zero, addr, ctx->mem_idx, MO_UQ);
     }
 }