qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH target-arm v1 1/1] arm: translate.c: Fix smlald


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH target-arm v1 1/1] arm: translate.c: Fix smlald Instruction
Date: Thu, 17 Apr 2014 13:22:06 +1000

On Wed, Apr 16, 2014 at 1:27 AM, Peter Maydell <address@hidden> wrote:
> On 4 April 2014 03:19, Peter Crosthwaite <address@hidden> wrote:
>> The smlald (and probably smlsld) instruction was doing incorrect sign
>> extensions of the operands amongst 64bit result calculation. The
>> instruction psuedo-code is:
>>
>>  operand2 = if m_swap then ROR(R[m],16) else R[m];
>>  product1 = SInt(R[n]<15:0>) * SInt(operand2<15:0>);
>>  product2 = SInt(R[n]<31:16>) * SInt(operand2<31:16>);
>>  result = product1 + product2 + SInt(R[dHi]:R[dLo]);
>>  R[dHi] = result<63:32>;
>>  R[dLo] = result<31:0>;
>>
>> The result calculation should be done in 64 bit arithmetic, and hence
>> product1 and product2 should be sign extended to 64b before calculation.
>>
>> The current implementation was adding product1 and product2 together
>> then sign-extending the intermediate result leading to false negatives.
>>
>> E.G. if product1 = product2 = 0x4000000, their sum = 0x80000000, which
>> will be incorrectly interpreted as -ve on sign extension.
>>
>> We fix by doing the 64b extensions on both product1 and product2 before
>> any addition/subtraction happens.
>
> Yes, this looks right. You also fix that we were
> possibly incorrectly setting the Q saturation flag for
> SMLSLD, which the ARM ARM specifically says is not set:
> can you mention that in the commit message too?
>

Yes. I've taken what you have said there near verbatim to commit
message (s/you/we).

> Interestingly, my random-instruction-set testing doesn't
> notice this bug: it must just never manage to hit a pair
> of input values which trigger it.
>
>> Reported-by: Christina Smith <address@hidden>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  target-arm/translate.c | 33 ++++++++++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 56e3b4b..5a62b54 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -7278,6 +7278,7 @@ static void disas_arm_insn(CPUARMState * env, 
>> DisasContext *s)
>>      TCGv_i32 tmp3;
>>      TCGv_i32 addr;
>>      TCGv_i64 tmp64;
>> +    TCGv_i64 tmp64_2;
>
> Can you declare this more locally to where it's used,
> please?
>

Done.

>>
>>      insn = arm_ldl_code(env, s->pc, s->bswap_code);
>>      s->pc += 4;
>> @@ -8328,26 +8329,36 @@ static void disas_arm_insn(CPUARMState * env, 
>> DisasContext *s)
>>                          if (insn & (1 << 5))
>>                              gen_swap_half(tmp2);
>>                          gen_smul_dual(tmp, tmp2);
>> -                        if (insn & (1 << 6)) {
>> -                            /* This subtraction cannot overflow. */
>> -                            tcg_gen_sub_i32(tmp, tmp, tmp2);
>> -                        } else {
>> -                            /* This addition cannot overflow 32 bits;
>> -                             * however it may overflow considered as a 
>> signed
>> -                             * operation, in which case we must set the Q 
>> flag.
>> -                             */
>> -                            gen_helper_add_setq(tmp, cpu_env, tmp, tmp2);
>> -                        }
>> -                        tcg_temp_free_i32(tmp2);
>>                          if (insn & (1 << 22)) {
>>                              /* smlald, smlsld */
>>                              tmp64 = tcg_temp_new_i64();
>> +                            tmp64_2 = tcg_temp_new_i64();
>>                              tcg_gen_ext_i32_i64(tmp64, tmp);
>> +                            tcg_gen_ext_i32_i64(tmp64_2, tmp2);
>>                              tcg_temp_free_i32(tmp);
>> +                            tcg_temp_free_i32(tmp2);
>> +                            if (insn & (1 << 6)) {
>> +                                tcg_gen_sub_i64(tmp64, tmp64, tmp64_2);
>> +                            } else {
>> +                                tcg_gen_add_i64(tmp64, tmp64, tmp64_2);
>> +                            }
>> +                            tcg_temp_free_i64(tmp64_2);
>>                              gen_addq(s, tmp64, rd, rn);
>>                              gen_storeq_reg(s, rd, rn, tmp64);
>>                              tcg_temp_free_i64(tmp64);
>>                          } else {
>> +                            if (insn & (1 << 6)) {
>> +                                /* This subtraction cannot overflow. */
>> +                                tcg_gen_sub_i32(tmp, tmp, tmp2);
>> +                            } else {
>> +                                /* This addition cannot overflow 32 bits;
>> +                                 * however it may overflow considered as a
>> +                                 * signed operation, in which case we must 
>> set
>> +                                 * the Q flag.
>> +                                 */
>> +                                gen_helper_add_setq(tmp, cpu_env, tmp, 
>> tmp2);
>> +                            }
>> +                            tcg_temp_free_i32(tmp2);
>>                              /* smuad, smusd, smlad, smlsd */
>
> This comment should go above the chunk of code you've just moved
> into this else {}, not below it.
>

Fixed.

>>                              if (rd != 15)
>>                                {
>
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
>

Thanks. All corrections made and V2 on list.

Regards,
Peter

> thanks
> -- PMM
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]