On Wed, Apr 16, 2014 at 1:27 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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?
>
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 <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?
>
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 <peter.mayd...@linaro.org>
>
Thanks. All corrections made and V2 on list.
Regards,
Peter
> thanks
> -- PMM
>
