On 4 April 2014 03:19, Peter Crosthwaite <peter.crosthwa...@xilinx.com> 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 <christina.sm...@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
> ---
>
> 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 <peter.mayd...@linaro.org>
thanks
-- PMM
