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

[Peter Maydell]

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?

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?

>
>      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.

>                              if (rd != 15)
>                                {

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM