On Wed, Jan 05, 2011 at 11:15:15AM +0000, Peter Maydell wrote:
> On 1 January 2011 18:25, Aurelien Jarno <aurel...@aurel32.net> wrote:
> > SMMLA and SMMLS are broken on both in normal and thumb mode, that is
> > both (different) implementations are wrong. They try to avoid a 64-bit
> > add for the rounding, which is not trivial if you want to support both
> > SMMLA and SMMLS with the same code.
> >
> > The code below uses the same implementation for both modes, using the
> > code from the ARM manual. It also fixes the thumb decoding that was a
> > mix between normal and thumb mode.
> >
> > This fixes the issues reported in
> > https://bugs.launchpad.net/qemu/+bug/629298
>
> I've tested this patch with my random-sequence-generator for
> SMMLA/SMMLS/SMMUL for ARM and Thumb, and it does fix
> the bug. I have a few minor nitpicks about some comments, though.
>
> > -/* Round the top 32 bits of a 64-bit value. */
> > -static void gen_roundqd(TCGv a, TCGv b)
> > +/* Add a to the msw of b. Mark inputs as dead */
> > +static TCGv_i64 gen_addq_msw(TCGv_i64 a, TCGv b)
> > {
> > - tcg_gen_shri_i32(a, a, 31);
> > - tcg_gen_add_i32(a, a, b);
> > + TCGv_i64 tmp64 = tcg_temp_new_i64();
> > +
> > + tcg_gen_extu_i32_i64(tmp64, b);
> > + dead_tmp(b);
> > + tcg_gen_shli_i64(tmp64, tmp64, 32);
> > + tcg_gen_add_i64(a, tmp64, a);
> > +
> > + tcg_temp_free_i64(tmp64);
> > + return a;
> > +}
>
> Isn't this adding b to the msw of a, rather than the other
> way round as the comment claims?
I think the comment is actually wrong in both way, as a shift is
applied, and thus lsw of b is used as the msw in the addition.
What about "Add a to (b << 32). Mark inputs as dead."?
> > +/* Subtract a from the msw of b. Mark inputs as dead. */
>
> Ditto.
What about "subtract a from (b << 32). Mark inputs as dead.".
> > @@ -6953,23 +6958,25 @@ static void disas_arm_insn(CPUState * env,
> > DisasContext *s)
> > tmp = load_reg(s, rm);
> > tmp2 = load_reg(s, rs);
> > if (insn & (1 << 20)) {
> > - /* Signed multiply most significant [accumulate].
> > */
> > + /* Signed multiply most significant [accumulate].
> > + (SMMUL, SMLA, SMMLS) */
>
> SMMLA, not SMLA.
>
I'll fix that in the next version.
Thanks for the review.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net
