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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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);
>