[Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Stefan Weil]

The first patch was triggered by a compilation error with configuration
--enable-debug-tcg on 64 bit hosts.

While looking for a fix, I noticed another issue in the same function
which I tried to fix in the 2nd patch.

Finally, I reviewed all allocations of TCGv_i64 values and the
matching tcg_temp_free_i64() calls.

Please review my patches - I only did a compile check on x86 linux.

Feel free to combine patches if larger patches are preferred.

Regards,
Stefan W.

[PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()
[PATCH 02/12] target-s390x: Fix duplicate call of tcg_temp_new_i64
[PATCH 03/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 04/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 05/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 06/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 07/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 08/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 09/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 10/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 11/12] target-s390x: Add missing tcg_temp_free_i64()
[PATCH 12/12] target-s390x: Add missing tcg_temp_free_i32()

[Qemu-devel] [PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Stefan Weil]

tcg_gen_shl_i64 needs an argument of type TCGv_i64.
Using tmp4 needs some additional changes.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 8e71df3..3614516 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2056,7 +2056,6 @@ do_mh:
            even for very long ones... */
         tmp = get_address(s, 0, b2, d2);
         tmp3 = tcg_const_i64(stm_len);
-        tmp4 = tcg_const_i64(32);
         for (i = r1;; i = (i + 1) % 16) {
             switch (op) {
             case 0x4:
@@ -2070,7 +2069,9 @@ do_mh:
 #else
                 tmp2 = tcg_temp_new_i64();
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
-                tcg_gen_shl_i64(tmp2, tmp2, 4);
+                tmp4 = tcg_const_i64(4);
+                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
+                tcg_temp_free_i64(tmp4);
                 tcg_gen_ext32u_i64(regs[i], regs[i]);
                 tcg_gen_or_i64(regs[i], regs[i], tmp2);
 #endif
@@ -2081,7 +2082,9 @@ do_mh:
                 break;
             case 0x26:
                 tmp2 = tcg_temp_new_i64();
+                tmp4 = tcg_const_i64(32);
                 tcg_gen_shr_i64(tmp2, regs[i], tmp4);
+                tcg_temp_free_i64(tmp4);
                 tcg_gen_qemu_st32(tmp2, tmp, get_mem_index(s));
                 tcg_temp_free_i64(tmp2);
                 break;
@@ -2094,7 +2097,6 @@ do_mh:
             tcg_gen_add_i64(tmp, tmp, tmp3);
         }
         tcg_temp_free_i64(tmp);
-        tcg_temp_free_i64(tmp4);
         break;
     case 0x2c: /* STCMH R1,M3,D2(B2) [RSY] */
         tmp = get_address(s, 0, b2, d2);
-- 
1.7.2.5

[Qemu-devel] [PATCH 02/12] target-s390x: Fix duplicate call of tcg_temp_new_i64

[Stefan Weil]

tmp2 = tcg_temp_new_i64() is already executed unconditionally,
so there is no need to call it a second time for 64 bit hosts.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 3614516..a240efa 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2067,7 +2067,6 @@ do_mh:
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
                 tcg_gen_trunc_i64_i32(TCGV_HIGH(regs[i]), tmp2);
 #else
-                tmp2 = tcg_temp_new_i64();
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
                 tmp4 = tcg_const_i64(4);
                 tcg_gen_shl_i64(tmp2, tmp2, tmp4);
-- 
1.7.2.5

[Qemu-devel] [PATCH 03/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index a240efa..bf6bda5 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1095,6 +1095,7 @@ static void gen_jcc(DisasContext *s, uint32_t mask, int skip)
             tcg_gen_brcondi_i64(TCG_COND_EQ, tmp64, 0, skip);
             break;
         default:
+            tcg_temp_free_i64(tmp64);
             goto do_dynamic;
         }
         tcg_temp_free_i64(tmp64);
-- 
1.7.2.5

[Qemu-devel] [PATCH 04/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index bf6bda5..723e53d 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2097,6 +2097,7 @@ do_mh:
             tcg_gen_add_i64(tmp, tmp, tmp3);
         }
         tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp3);
         break;
     case 0x2c: /* STCMH R1,M3,D2(B2) [RSY] */
         tmp = get_address(s, 0, b2, d2);
-- 
1.7.2.5

[Qemu-devel] [PATCH 05/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 723e53d..bf6a363 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2966,6 +2966,8 @@ static void disas_b2(DisasContext *s, int op, uint32_t insn)
         /* we need to keep cc_op intact */
         s->is_jmp = DISAS_JUMP;
         tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp2);
+        tcg_temp_free_i64(tmp3);
         break;
     case 0x20: /* SERVC     R1,R2     [RRE] */
         /* SCLP Service call (PV hypercall) */
-- 
1.7.2.5

[Qemu-devel] [PATCH 06/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index bf6a363..474d13b 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -4598,6 +4598,8 @@ static void disas_s390_insn(DisasContext *s)
         store_reg32(r1, tmp32_1);
         tcg_gen_trunc_i64_i32(tmp32_2, tmp2);
         store_reg32(r1 + 1, tmp32_2);
+        tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp2);
         break;
     case 0x98: /* LM     R1,R3,D2(B2)     [RS] */
     case 0x90: /* STM    R1,R3,D2(B2)     [RS] */
-- 
1.7.2.5

[Qemu-devel] [PATCH 07/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 474d13b..3143703 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -4623,6 +4623,7 @@ static void disas_s390_insn(DisasContext *s)
             }
             tcg_gen_add_i64(tmp, tmp, tmp3);
         }
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i64(tmp3);
         tcg_temp_free_i64(tmp4);
-- 
1.7.2.5

[Qemu-devel] [PATCH 08/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 3143703..aec1272 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2367,6 +2367,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_shri_i64(tmp2, tmp, 48);
         tcg_gen_trunc_i64_i32(tmp32, tmp2);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH 09/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index aec1272..67b249b 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2393,6 +2393,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp2);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH 10/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 67b249b..cfd2297 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2419,6 +2419,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH 11/12] target-s390x: Add missing tcg_temp_free_i64()

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index cfd2297..1261be4 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2443,6 +2443,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);        /* signedness should not matter here */
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH 12/12] target-s390x: Add missing tcg_temp_free_i32()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 1261be4..baecd3f 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1078,9 +1078,12 @@ static void gen_jcc(DisasContext *s, uint32_t mask, int skip)
             tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, skip);
             break;
         default:
+            tcg_temp_free_i32(tmp);
+            tcg_temp_free_i32(tmp2);
             goto do_dynamic;
         }
         tcg_temp_free_i32(tmp);
+        tcg_temp_free_i32(tmp2);
         account_inline_branch(s);
         break;
     case CC_OP_TM_64:
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Peter Maydell]

On 25 May 2011 21:25, Stefan Weil <weil@mail.berlios.de> wrote:
> Feel free to combine patches if larger patches are preferred.

I'd vote for combining at least 03..12, having nine patches all
of which have the same summary line is a bit confusing :-)

-- PMM

Re: [Qemu-devel] [PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Alexander Graf]

On 25.05.2011, at 22:25, Stefan Weil wrote:

> tcg_gen_shl_i64 needs an argument of type TCGv_i64.
> Using tmp4 needs some additional changes.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
> target-s390x/translate.c |    8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 8e71df3..3614516 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -2056,7 +2056,6 @@ do_mh:
>            even for very long ones... */
>         tmp = get_address(s, 0, b2, d2);
>         tmp3 = tcg_const_i64(stm_len);
> -        tmp4 = tcg_const_i64(32);
>         for (i = r1;; i = (i + 1) % 16) {
>             switch (op) {
>             case 0x4:
> @@ -2070,7 +2069,9 @@ do_mh:
> #else
>                 tmp2 = tcg_temp_new_i64();
>                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
> -                tcg_gen_shl_i64(tmp2, tmp2, 4);
> +                tmp4 = tcg_const_i64(4);
> +                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
> +                tcg_temp_free_i64(tmp4);
>                 tcg_gen_ext32u_i64(regs[i], regs[i]);
>                 tcg_gen_or_i64(regs[i], regs[i], tmp2);
> #endif
> @@ -2081,7 +2082,9 @@ do_mh:
>                 break;
>             case 0x26:
>                 tmp2 = tcg_temp_new_i64();
> +                tmp4 = tcg_const_i64(32);

This moves the const inside the loop, which is exactly what I was trying to avoid here. The problem is that every new const generated here issues 1 additional tcg op, which really sums up when there's too many of them. I've had the buffer exceed here plenty of times.


Alex

Re: [Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Alexander Graf]

On 26.05.2011, at 00:10, Peter Maydell wrote:

> On 25 May 2011 21:25, Stefan Weil <weil@mail.berlios.de> wrote:
>> Feel free to combine patches if larger patches are preferred.
> 
> I'd vote for combining at least 03..12, having nine patches all
> of which have the same summary line is a bit confusing :-)

I actually like them as individual patches. Makes bisecting a lot easier :).


Alex

Re: [Qemu-devel] [PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Stefan Weil]

Am 26.05.2011 00:15, schrieb Alexander Graf:
> On 25.05.2011, at 22:25, Stefan Weil wrote:
>
>> tcg_gen_shl_i64 needs an argument of type TCGv_i64.
>> Using tmp4 needs some additional changes.
>>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> ---
>> target-s390x/translate.c |    8 +++++---
>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
>> index 8e71df3..3614516 100644
>> --- a/target-s390x/translate.c
>> +++ b/target-s390x/translate.c
>> @@ -2056,7 +2056,6 @@ do_mh:
>>             even for very long ones... */
>>          tmp = get_address(s, 0, b2, d2);
>>          tmp3 = tcg_const_i64(stm_len);
>> -        tmp4 = tcg_const_i64(32);
>>          for (i = r1;; i = (i + 1) % 16) {
>>              switch (op) {
>>              case 0x4:
>> @@ -2070,7 +2069,9 @@ do_mh:
>> #else
>>                  tmp2 = tcg_temp_new_i64();
>>                  tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
>> -                tcg_gen_shl_i64(tmp2, tmp2, 4);
>> +                tmp4 = tcg_const_i64(4);
>> +                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
>> +                tcg_temp_free_i64(tmp4);
>>                  tcg_gen_ext32u_i64(regs[i], regs[i]);
>>                  tcg_gen_or_i64(regs[i], regs[i], tmp2);
>> #endif
>> @@ -2081,7 +2082,9 @@ do_mh:
>>                  break;
>>              case 0x26:
>>                  tmp2 = tcg_temp_new_i64();
>> +                tmp4 = tcg_const_i64(32);
> This moves the const inside the loop, which is exactly what I was trying to avoid here. The problem is that every new const generated here issues 1 additional tcg op, which really sums up when there's too many of them. I've had the buffer exceed here plenty of times.
>
> Alex

I noticed that, too. But adding a tmp5 and generating two const outside 
the loop
of whom only one or even none is used is also a bad solution.

What about moving the loop inside the switch statement (one loop for 
every case)?
This does not look nice but looks like the best solution here.

If you prefer a different solution, just omit this part of my patch series.

Grüße, Stefan

Re: [Qemu-devel] [PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Stefan Weil]

Am 26.05.2011 06:32, schrieb Stefan Weil:
> Am 26.05.2011 00:15, schrieb Alexander Graf:
>> On 25.05.2011, at 22:25, Stefan Weil wrote:
>>
>>> tcg_gen_shl_i64 needs an argument of type TCGv_i64.
>>> Using tmp4 needs some additional changes.
>>>
>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>> ---
>>> target-s390x/translate.c |    8 +++++---
>>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
>>> index 8e71df3..3614516 100644
>>> --- a/target-s390x/translate.c
>>> +++ b/target-s390x/translate.c
>>> @@ -2056,7 +2056,6 @@ do_mh:
>>>             even for very long ones... */
>>>          tmp = get_address(s, 0, b2, d2);
>>>          tmp3 = tcg_const_i64(stm_len);
>>> -        tmp4 = tcg_const_i64(32);
>>>          for (i = r1;; i = (i + 1) % 16) {
>>>              switch (op) {
>>>              case 0x4:
>>> @@ -2070,7 +2069,9 @@ do_mh:
>>> #else
>>>                  tmp2 = tcg_temp_new_i64();
>>>                  tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
>>> -                tcg_gen_shl_i64(tmp2, tmp2, 4);
>>> +                tmp4 = tcg_const_i64(4);
>>> +                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
>>> +                tcg_temp_free_i64(tmp4);
>>>                  tcg_gen_ext32u_i64(regs[i], regs[i]);
>>>                  tcg_gen_or_i64(regs[i], regs[i], tmp2);
>>> #endif
>>> @@ -2081,7 +2082,9 @@ do_mh:
>>>                  break;
>>>              case 0x26:
>>>                  tmp2 = tcg_temp_new_i64();
>>> +                tmp4 = tcg_const_i64(32);
>> This moves the const inside the loop, which is exactly what I was 
>> trying to avoid here. The problem is that every new const generated 
>> here issues 1 additional tcg op, which really sums up when there's 
>> too many of them. I've had the buffer exceed here plenty of times.
>>
>> Alex
>
> I noticed that, too. But adding a tmp5 and generating two const 
> outside the loop
> of whom only one or even none is used is also a bad solution.
>
> What about moving the loop inside the switch statement (one loop for 
> every case)?
> This does not look nice but looks like the best solution here.
>
> If you prefer a different solution, just omit this part of my patch 
> series.
>
> Grüße, Stefan

Would it be possible to use tcg_gen_shli_64 / tcg_gen_shri_64 and remove
tmp4 completely?

Stefan

Re: [Qemu-devel] [PATCH 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Alexander Graf]

On 26.05.2011, at 06:56, Stefan Weil wrote:

> Am 26.05.2011 06:32, schrieb Stefan Weil:
>> Am 26.05.2011 00:15, schrieb Alexander Graf:
>>> On 25.05.2011, at 22:25, Stefan Weil wrote:
>>> 
>>>> tcg_gen_shl_i64 needs an argument of type TCGv_i64.
>>>> Using tmp4 needs some additional changes.
>>>> 
>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>>> ---
>>>> target-s390x/translate.c |    8 +++++---
>>>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
>>>> index 8e71df3..3614516 100644
>>>> --- a/target-s390x/translate.c
>>>> +++ b/target-s390x/translate.c
>>>> @@ -2056,7 +2056,6 @@ do_mh:
>>>>            even for very long ones... */
>>>>         tmp = get_address(s, 0, b2, d2);
>>>>         tmp3 = tcg_const_i64(stm_len);
>>>> -        tmp4 = tcg_const_i64(32);
>>>>         for (i = r1;; i = (i + 1) % 16) {
>>>>             switch (op) {
>>>>             case 0x4:
>>>> @@ -2070,7 +2069,9 @@ do_mh:
>>>> #else
>>>>                 tmp2 = tcg_temp_new_i64();
>>>>                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
>>>> -                tcg_gen_shl_i64(tmp2, tmp2, 4);
>>>> +                tmp4 = tcg_const_i64(4);
>>>> +                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
>>>> +                tcg_temp_free_i64(tmp4);
>>>>                 tcg_gen_ext32u_i64(regs[i], regs[i]);
>>>>                 tcg_gen_or_i64(regs[i], regs[i], tmp2);
>>>> #endif
>>>> @@ -2081,7 +2082,9 @@ do_mh:
>>>>                 break;
>>>>             case 0x26:
>>>>                 tmp2 = tcg_temp_new_i64();
>>>> +                tmp4 = tcg_const_i64(32);
>>> This moves the const inside the loop, which is exactly what I was trying to avoid here. The problem is that every new const generated here issues 1 additional tcg op, which really sums up when there's too many of them. I've had the buffer exceed here plenty of times.
>>> 
>>> Alex
>> 
>> I noticed that, too. But adding a tmp5 and generating two const outside the loop
>> of whom only one or even none is used is also a bad solution.
>> 
>> What about moving the loop inside the switch statement (one loop for every case)?
>> This does not look nice but looks like the best solution here.
>> 
>> If you prefer a different solution, just omit this part of my patch series.
>> 
>> Grüße, Stefan
> 
> Would it be possible to use tcg_gen_shli_64 / tcg_gen_shri_64 and remove
> tmp4 completely?

That's what I had originally. It basically does exactly the same as the code you have patched it to :). Any immediate tcg op internally just generates a const tcg var and calls the generic one.


Alex

Re: [Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Andreas Färber]

Am 26.05.2011 um 00:17 schrieb Alexander Graf:

> On 26.05.2011, at 00:10, Peter Maydell wrote:
>
>> On 25 May 2011 21:25, Stefan Weil <weil@mail.berlios.de> wrote:
>>> Feel free to combine patches if larger patches are preferred.
>>
>> I'd vote for combining at least 03..12, having nine patches all
>> of which have the same summary line is a bit confusing :-)
>
> I actually like them as individual patches. Makes bisecting a lot  
> easier :).

Stefan, could you add the function name or some other discriminator to  
the subject then, please?

Andreas

Re: [Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Stefan Weil]

Am 26.05.2011 23:48, schrieb Andreas Färber:
> Am 26.05.2011 um 00:17 schrieb Alexander Graf:
>
>> On 26.05.2011, at 00:10, Peter Maydell wrote:
>>
>>> On 25 May 2011 21:25, Stefan Weil <weil@mail.berlios.de> wrote:
>>>> Feel free to combine patches if larger patches are preferred.
>>>
>>> I'd vote for combining at least 03..12, having nine patches all
>>> of which have the same summary line is a bit confusing :-)
>>
>> I actually like them as individual patches. Makes bisecting a lot 
>> easier :).
>
> Stefan, could you add the function name or some other discriminator to 
> the subject then, please?
>
> Andreas

This would result in these subjects:

4 x target-s390x: Add missing tcg_temp_free_i64() in disas_a5
2 x target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn
1 x target-s390x: Add missing tcg_temp_free_i64() in disas_b2
1 x target-s390x: Add missing tcg_temp_free_i64() in do_mh
1 x target-s390x: Add missing tcg_temp_free_i64() in gen_jcc

So there remain duplicate subjects. Adding the line number or a sequence 
number
would look somehow strange.

The subjects are identical simply because the changes are very similar.
If needed, git provides the unique identification.

Maybe combining the patches which share the same function and add
this function name to the subject would be a compromise which still
allows bisecting.

Cheers,
Stefan

Re: [Qemu-devel] [PATCH 00/12] target-s390x: Several small fixes

[Alexander Graf]

On 27.05.2011, at 06:59, Stefan Weil wrote:

> Am 26.05.2011 23:48, schrieb Andreas Färber:
>> Am 26.05.2011 um 00:17 schrieb Alexander Graf:
>> 
>>> On 26.05.2011, at 00:10, Peter Maydell wrote:
>>> 
>>>> On 25 May 2011 21:25, Stefan Weil <weil@mail.berlios.de> wrote:
>>>>> Feel free to combine patches if larger patches are preferred.
>>>> 
>>>> I'd vote for combining at least 03..12, having nine patches all
>>>> of which have the same summary line is a bit confusing :-)
>>> 
>>> I actually like them as individual patches. Makes bisecting a lot easier :).
>> 
>> Stefan, could you add the function name or some other discriminator to the subject then, please?
>> 
>> Andreas
> 
> This would result in these subjects:
> 
> 4 x target-s390x: Add missing tcg_temp_free_i64() in disas_a5
> 2 x target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn

Inside the functions there's a switch that determines the actual opcode. Just use that in the name?


Alex

[Qemu-devel] [PATCH v2 00/12] target-s390x: Several small fixes

[Stefan Weil]

This is an update of my last patch series.

Modifications in v2:

* Changed 01/12 to create more efficient code.
* Modified subject lines of 03/12 up to 11/12 so there are no duplicates.

Regards,
Stefan W.

[PATCH v2 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()
[PATCH v2 02/12] target-s390x: Fix duplicate call of tcg_temp_new_i64
[PATCH v2 03/12] target-s390x: Add missing tcg_temp_free_i64() in gen_jcc()
[PATCH v2 04/12] target-s390x: Add missing tcg_temp_free_i64() in do_mh()
[PATCH v2 05/12] target-s390x: Add missing tcg_temp_free_i64() in disas_b2()
[PATCH v2 06/12] target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn(), opc == 0x8e
[PATCH v2 07/12] target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn(), opc == 0x90
[PATCH v2 08/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0x8
[PATCH v2 09/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0x9
[PATCH v2 10/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0xa
[PATCH v2 11/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0xb
[PATCH v2 12/12] target-s390x: Add missing tcg_temp_free_i32()

[Qemu-devel] [PATCH v2 01/12] target-s390x: Fix wrong argument in call of tcg_gen_shl_i64()

[Stefan Weil]

tcg_gen_shl_i64 needs a 3rd argument of type TCGv_i64.
Set tmp4 so it can be used here.

v2:
Don't call tcg_const_i64() inside of the loop
because it creates additional code.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 8e71df3..865a9df 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2056,7 +2056,7 @@ do_mh:
            even for very long ones... */
         tmp = get_address(s, 0, b2, d2);
         tmp3 = tcg_const_i64(stm_len);
-        tmp4 = tcg_const_i64(32);
+        tmp4 = tcg_const_i64(op == 0x26 ? 32 : 4);
         for (i = r1;; i = (i + 1) % 16) {
             switch (op) {
             case 0x4:
@@ -2070,7 +2070,7 @@ do_mh:
 #else
                 tmp2 = tcg_temp_new_i64();
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
-                tcg_gen_shl_i64(tmp2, tmp2, 4);
+                tcg_gen_shl_i64(tmp2, tmp2, tmp4);
                 tcg_gen_ext32u_i64(regs[i], regs[i]);
                 tcg_gen_or_i64(regs[i], regs[i], tmp2);
 #endif
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 02/12] target-s390x: Fix duplicate call of tcg_temp_new_i64

[Stefan Weil]

tmp2 = tcg_temp_new_i64() is already executed unconditionally,
so there is no need to call it a second time for 64 bit hosts.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 865a9df..141a72f 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2068,7 +2068,6 @@ do_mh:
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
                 tcg_gen_trunc_i64_i32(TCGV_HIGH(regs[i]), tmp2);
 #else
-                tmp2 = tcg_temp_new_i64();
                 tcg_gen_qemu_ld32u(tmp2, tmp, get_mem_index(s));
                 tcg_gen_shl_i64(tmp2, tmp2, tmp4);
                 tcg_gen_ext32u_i64(regs[i], regs[i]);
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 03/12] target-s390x: Add missing tcg_temp_free_i64() in gen_jcc()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 141a72f..6ec77ec 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1095,6 +1095,7 @@ static void gen_jcc(DisasContext *s, uint32_t mask, int skip)
             tcg_gen_brcondi_i64(TCG_COND_EQ, tmp64, 0, skip);
             break;
         default:
+            tcg_temp_free_i64(tmp64);
             goto do_dynamic;
         }
         tcg_temp_free_i64(tmp64);
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 04/12] target-s390x: Add missing tcg_temp_free_i64() in do_mh()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 6ec77ec..a11cb19 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2094,6 +2094,7 @@ do_mh:
             tcg_gen_add_i64(tmp, tmp, tmp3);
         }
         tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp3);
         tcg_temp_free_i64(tmp4);
         break;
     case 0x2c: /* STCMH R1,M3,D2(B2) [RSY] */
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 05/12] target-s390x: Add missing tcg_temp_free_i64() in disas_b2()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index a11cb19..f3f42a9 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2964,6 +2964,8 @@ static void disas_b2(DisasContext *s, int op, uint32_t insn)
         /* we need to keep cc_op intact */
         s->is_jmp = DISAS_JUMP;
         tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp2);
+        tcg_temp_free_i64(tmp3);
         break;
     case 0x20: /* SERVC     R1,R2     [RRE] */
         /* SCLP Service call (PV hypercall) */
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 06/12] target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn(), opc == 0x8e

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index f3f42a9..c5a3930 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -4596,6 +4596,8 @@ static void disas_s390_insn(DisasContext *s)
         store_reg32(r1, tmp32_1);
         tcg_gen_trunc_i64_i32(tmp32_2, tmp2);
         store_reg32(r1 + 1, tmp32_2);
+        tcg_temp_free_i64(tmp);
+        tcg_temp_free_i64(tmp2);
         break;
     case 0x98: /* LM     R1,R3,D2(B2)     [RS] */
     case 0x90: /* STM    R1,R3,D2(B2)     [RS] */
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 07/12] target-s390x: Add missing tcg_temp_free_i64() in disas_s390_insn(), opc == 0x90

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index c5a3930..81b8c5b 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -4621,6 +4621,7 @@ static void disas_s390_insn(DisasContext *s)
             }
             tcg_gen_add_i64(tmp, tmp, tmp3);
         }
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i64(tmp3);
         tcg_temp_free_i64(tmp4);
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 08/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0x8

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 81b8c5b..692de6e 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2365,6 +2365,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_shri_i64(tmp2, tmp, 48);
         tcg_gen_trunc_i64_i32(tmp32, tmp2);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 09/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0x9

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 692de6e..705fe2b 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2391,6 +2391,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp2);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 10/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0xa

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 705fe2b..4f4b893 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2417,6 +2417,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 11/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0xb

[Stefan Weil]

load_reg() needs a matching tcg_temp_free_i64().

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 4f4b893..6664ab5 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2441,6 +2441,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
         tcg_gen_trunc_i64_i32(tmp32, tmp);
         tcg_gen_andi_i32(tmp32, tmp32, 0xffff);
         set_cc_nz_u32(s, tmp32);        /* signedness should not matter here */
+        tcg_temp_free_i64(tmp);
         tcg_temp_free_i64(tmp2);
         tcg_temp_free_i32(tmp32);
         break;
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 12/12] target-s390x: Add missing tcg_temp_free_i32()

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 6664ab5..0269970 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1078,9 +1078,12 @@ static void gen_jcc(DisasContext *s, uint32_t mask, int skip)
             tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, skip);
             break;
         default:
+            tcg_temp_free_i32(tmp);
+            tcg_temp_free_i32(tmp2);
             goto do_dynamic;
         }
         tcg_temp_free_i32(tmp);
+        tcg_temp_free_i32(tmp2);
         account_inline_branch(s);
         break;
     case CC_OP_TM_64:
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2 08/12] target-s390x: Add missing tcg_temp_free_i64() in disas_a5(), opc == 0x8

[Alexander Graf]

On 27.05.2011, at 19:03, Stefan Weil wrote:

> load_reg() needs a matching tcg_temp_free_i64().
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
> target-s390x/translate.c |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 81b8c5b..692de6e 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -2365,6 +2365,7 @@ static void disas_a5(DisasContext *s, int op, int r1, int i2)
>         tcg_gen_shri_i64(tmp2, tmp, 48);
>         tcg_gen_trunc_i64_i32(tmp32, tmp2);
>         set_cc_nz_u32(s, tmp32);
> +        tcg_temp_free_i64(tmp);

tmp gets freed at the end of the function, so this one is bad.


Alex