[v2] target/s390x: pc-relative translation

[PATCH v2 06/10] target/s390x: Use deposit in save_link_info

Posted by Richard Henderson 1 year, 8 months ago

Replace manual masking and oring with deposits.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 2654c85a8e..0f0688424f 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1418,24 +1418,32 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 t;
+    TCGv_i64 t1, t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
         pc_to_link_info(o->out, s);
         return;
     }
+
     gen_op_calc_cc(s);
-    t = tcg_temp_new_i64();
-    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-    gen_psw_addr_disp(s, t, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t, psw_mask, 16);
-    tcg_gen_andi_i64(t, t, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_extu_i32_i64(t, cc_op);
-    tcg_gen_shli_i64(t, t, 28);
-    tcg_gen_or_i64(o->out, o->out, t);
+    t1 = tcg_temp_new_i64();
+    t2 = tcg_temp_new_i64();
+
+    /* Shift program mask into place, garbage outside of [27:24]. */
+    tcg_gen_shri_i64(t1, psw_mask, 16);
+    /* Deposit pc to replace garbage bits below program mask. */
+    gen_psw_addr_disp(s, t2, s->ilen);
+    tcg_gen_deposit_i64(t1, t1, t2, 0, 24);
+    /*
+     * Deposit cc to replace garbage bits above program mask.
+     * Note that cc is in [0-3], thus [63:30] are set to zero.
+     */
+    tcg_gen_extu_i32_i64(t2, cc_op);
+    tcg_gen_deposit_i64(t1, t1, t2, 28, 64 - 28);
+    /* Install ilen. */
+    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
+
+    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.34.1

[PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

Posted by Philippe Mathieu-Daudé 1 year, 5 months ago

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 2/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index faa6d37c8e..53ec817e29 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
+    TCGv_i64 t1;
     TCGv_i64 t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
@@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     }
 
     gen_op_calc_cc(s);
+    t1 = tcg_temp_new_i64();
     t2 = tcg_temp_new_i64();
+
     tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
+
+    /* Shift program mask into place, garbage outside of [27:24]. */
+    tcg_gen_shri_i64(t1, psw_mask, 16);
+    /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t2);
+    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
     tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t2, psw_mask, 16);
-    tcg_gen_andi_i64(t2, t2, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t2);
     tcg_gen_extu_i32_i64(t2, cc_op);
     tcg_gen_shli_i64(t2, t2, 28);
     tcg_gen_or_i64(o->out, o->out, t2);
-- 
2.45.2

Re: [PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

Posted by Richard Henderson 1 year, 5 months ago

On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
> [PMD: Split patch, part 2/4]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index faa6d37c8e..53ec817e29 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
>   
>   static void save_link_info(DisasContext *s, DisasOps *o)
>   {
> +    TCGv_i64 t1;
>       TCGv_i64 t2;
>   
>       if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
> @@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, DisasOps *o)
>       }
>   
>       gen_op_calc_cc(s);
> +    t1 = tcg_temp_new_i64();
>       t2 = tcg_temp_new_i64();
> +
>       tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
> +
> +    /* Shift program mask into place, garbage outside of [27:24]. */
> +    tcg_gen_shri_i64(t1, psw_mask, 16);
> +    /* Deposit pc to replace garbage bits below program mask. */
>       gen_psw_addr_disp(s, t2, s->ilen);
> -    tcg_gen_or_i64(o->out, o->out, t2);
> +    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);

This is incorrect, as you've lost the high 32-bits of out.


r~


>       tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
> -    tcg_gen_shri_i64(t2, psw_mask, 16);
> -    tcg_gen_andi_i64(t2, t2, 0x0f000000);
> -    tcg_gen_or_i64(o->out, o->out, t2);
>       tcg_gen_extu_i32_i64(t2, cc_op);
>       tcg_gen_shli_i64(t2, t2, 28);
>       tcg_gen_or_i64(o->out, o->out, t2);

Re: [PATCH v2 06/10 2/4] target/s390x: Use deposit to set psw_mask in save_link_info

Posted by Philippe Mathieu-Daudé 1 year, 5 months ago

On 10/9/24 01:50, Richard Henderson wrote:
> On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
>> [PMD: Split patch, part 2/4]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/s390x/tcg/translate.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
>> index faa6d37c8e..53ec817e29 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -1417,6 +1417,7 @@ static DisasJumpType op_bas(DisasContext *s, 
>> DisasOps *o)
>>   static void save_link_info(DisasContext *s, DisasOps *o)
>>   {
>> +    TCGv_i64 t1;
>>       TCGv_i64 t2;
>>       if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
>> @@ -1425,14 +1426,17 @@ static void save_link_info(DisasContext *s, 
>> DisasOps *o)
>>       }
>>       gen_op_calc_cc(s);
>> +    t1 = tcg_temp_new_i64();
>>       t2 = tcg_temp_new_i64();
>> +
>>       tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
>> +
>> +    /* Shift program mask into place, garbage outside of [27:24]. */
>> +    tcg_gen_shri_i64(t1, psw_mask, 16);
>> +    /* Deposit pc to replace garbage bits below program mask. */
>>       gen_psw_addr_disp(s, t2, s->ilen);
>> -    tcg_gen_or_i64(o->out, o->out, t2);
>> +    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
> 
> This is incorrect, as you've lost the high 32-bits of out.

Ah, I felt something was not right but couldn't figure it out,
thanks for pointing at it.

The original patch is not trivial to review...

> 
> r~
> 
> 
>>       tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
>> -    tcg_gen_shri_i64(t2, psw_mask, 16);
>> -    tcg_gen_andi_i64(t2, t2, 0x0f000000);
>> -    tcg_gen_or_i64(o->out, o->out, t2);
>>       tcg_gen_extu_i32_i64(t2, cc_op);
>>       tcg_gen_shli_i64(t2, t2, 28);
>>       tcg_gen_or_i64(o->out, o->out, t2);
>

[PATCH v2 06/10 3/4] target/s390x: Use deposit to set ilen in save_link_info

Posted by Philippe Mathieu-Daudé 1 year, 5 months ago

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 3/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 53ec817e29..bfb7662329 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1429,17 +1429,19 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     t1 = tcg_temp_new_i64();
     t2 = tcg_temp_new_i64();
 
-    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-
     /* Shift program mask into place, garbage outside of [27:24]. */
     tcg_gen_shri_i64(t1, psw_mask, 16);
     /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
     tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
-    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
     tcg_gen_extu_i32_i64(t2, cc_op);
     tcg_gen_shli_i64(t2, t2, 28);
     tcg_gen_or_i64(o->out, o->out, t2);
+
+    /* Install ilen. */
+    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
+
+    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.45.2

Re: [PATCH v2 06/10 3/4] target/s390x: Use deposit to set ilen in save_link_info

Posted by Richard Henderson 1 year, 5 months ago

On 9/9/24 16:19, Philippe Mathieu-Daudé wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
> [PMD: Split patch, part 3/4]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 53ec817e29..bfb7662329 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1429,17 +1429,19 @@ static void save_link_info(DisasContext *s, DisasOps *o)
>       t1 = tcg_temp_new_i64();
>       t2 = tcg_temp_new_i64();
>   
> -    tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
> -
>       /* Shift program mask into place, garbage outside of [27:24]. */
>       tcg_gen_shri_i64(t1, psw_mask, 16);
>       /* Deposit pc to replace garbage bits below program mask. */
>       gen_psw_addr_disp(s, t2, s->ilen);
>       tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
> -    tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
>       tcg_gen_extu_i32_i64(t2, cc_op);
>       tcg_gen_shli_i64(t2, t2, 28);
>       tcg_gen_or_i64(o->out, o->out, t2);
> +
> +    /* Install ilen. */
> +    tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);

This is incorrect, as bits [63:28] of t1 are still garbage.

> +
> +    tcg_gen_deposit_i64(o->out, o->out, t1, 0, 32);

You've fixed the missing high bits of out, though.

r~

>   }
>   
>   static DisasJumpType op_bal(DisasContext *s, DisasOps *o)

[PATCH v2 06/10 4/4] target/s390x: Use deposit to set cc_op in save_link_info

Posted by Philippe Mathieu-Daudé 1 year, 5 months ago

From: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 4/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index bfb7662329..b9fdfdc85a 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1433,11 +1433,13 @@ static void save_link_info(DisasContext *s, DisasOps *o)
     tcg_gen_shri_i64(t1, psw_mask, 16);
     /* Deposit pc to replace garbage bits below program mask. */
     gen_psw_addr_disp(s, t2, s->ilen);
-    tcg_gen_deposit_i64(o->out, t1, t2, 0, 24);
+    tcg_gen_deposit_i64(t1, t1, t2, 0, 24);
+    /*
+     * Deposit cc to replace garbage bits above program mask.
+     * Note that cc is in [0-3], thus [63:30] are set to zero.
+     */
     tcg_gen_extu_i32_i64(t2, cc_op);
-    tcg_gen_shli_i64(t2, t2, 28);
-    tcg_gen_or_i64(o->out, o->out, t2);
-
+    tcg_gen_deposit_i64(t1, t1, t2, 28, 64 - 28);
     /* Install ilen. */
     tcg_gen_ori_i64(t1, t1, (s->ilen / 2) << 30);
 
-- 
2.45.2

[PATCH v2 06/10 1/4] target/s390x: Rename local variable in save_link_info

Posted by Philippe Mathieu-Daudé 1 year, 5 months ago

From: Richard Henderson <richard.henderson@linaro.org>

To simplify the following commits, rename 't' as 't2'.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20240605215739.4758-7-richard.henderson@linaro.org>
[PMD: Split patch, part 1/4]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/translate.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index e1b1dd43e1..faa6d37c8e 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1417,24 +1417,25 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
 
 static void save_link_info(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 t;
+    TCGv_i64 t2;
 
     if (s->base.tb->flags & (FLAG_MASK_32 | FLAG_MASK_64)) {
         pc_to_link_info(o->out, s);
         return;
     }
+
     gen_op_calc_cc(s);
-    t = tcg_temp_new_i64();
+    t2 = tcg_temp_new_i64();
     tcg_gen_andi_i64(o->out, o->out, 0xffffffff00000000ull);
-    gen_psw_addr_disp(s, t, s->ilen);
-    tcg_gen_or_i64(o->out, o->out, t);
+    gen_psw_addr_disp(s, t2, s->ilen);
+    tcg_gen_or_i64(o->out, o->out, t2);
     tcg_gen_ori_i64(o->out, o->out, (s->ilen / 2) << 30);
-    tcg_gen_shri_i64(t, psw_mask, 16);
-    tcg_gen_andi_i64(t, t, 0x0f000000);
-    tcg_gen_or_i64(o->out, o->out, t);
-    tcg_gen_extu_i32_i64(t, cc_op);
-    tcg_gen_shli_i64(t, t, 28);
-    tcg_gen_or_i64(o->out, o->out, t);
+    tcg_gen_shri_i64(t2, psw_mask, 16);
+    tcg_gen_andi_i64(t2, t2, 0x0f000000);
+    tcg_gen_or_i64(o->out, o->out, t2);
+    tcg_gen_extu_i32_i64(t2, cc_op);
+    tcg_gen_shli_i64(t2, t2, 28);
+    tcg_gen_or_i64(o->out, o->out, t2);
 }
 
 static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
-- 
2.45.2