Patchwork [1/8] ARM: Fix decoding of VFP forms of VCVT between float and int/fixed

login
register
mail settings
Submitter Peter Maydell
Date Nov. 11, 2010, 6:23 p.m.
Message ID <1289499842-28818-2-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/70851/
State New
Headers show

Comments

Peter Maydell - Nov. 11, 2010, 6:23 p.m.
Correct the decoding of source and destination registers
for the VFP forms of the VCVT instructions which convert
between floating point and integer or fixed-point.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)
Nathan Froyd - Dec. 6, 2010, 4:30 p.m.
On Thu, Nov 11, 2010 at 06:23:55PM +0000, Peter Maydell wrote:
> Correct the decoding of source and destination registers
> for the VFP forms of the VCVT instructions which convert
> between floating point and integer or fixed-point.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

I don't know how good QEMU's framework is, but it'd be nice to have your
testing code checked into the tree, in case anybody decides to do major
surgery on the ARM backend.

-Nathan
Peter Maydell - Dec. 6, 2010, 4:48 p.m.
On 6 December 2010 16:30, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Thu, Nov 11, 2010 at 06:23:55PM +0000, Peter Maydell wrote:
>> Correct the decoding of source and destination registers
>> for the VFP forms of the VCVT instructions which convert
>> between floating point and integer or fixed-point.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

Thanks. I'll resend this set with the softfloat-related comments addressed
for the two patches that affects (and the remaining unchanged patches
with your Reviewed-by: tag on them).

> I don't know how good QEMU's framework is, but it'd be nice to have your
> testing code checked into the tree, in case anybody decides to do major
> surgery on the ARM backend.

I'm not sure how well it would fit into being committed to qemu (yet):
it works as a program where you run half of it on real ARM hardware
and the other half under qemu (or valgrind) and it compares register
results after executing instructions by looking at the sigcontext struct
in a signal handler. Plus there's a perl script to generate random
instruction set sequences to feed the test program. It could be made
more automated and independent of having a reference bit of hardware
but I haven't got round to that yet. (Also since it has utility outside of
just testing qemu I'm not sure if it really belongs in the qemu repo.)

-- PMM
Nathan Froyd - Dec. 6, 2010, 4:57 p.m.
On Mon, Dec 06, 2010 at 04:48:25PM +0000, Peter Maydell wrote:
> I'm not sure how well it would fit into being committed to qemu (yet):
> it works as a program where you run half of it on real ARM hardware
> and the other half under qemu (or valgrind) and it compares register
> results after executing instructions by looking at the sigcontext struct
> in a signal handler. Plus there's a perl script to generate random
> instruction set sequences to feed the test program. It could be made
> more automated and independent of having a reference bit of hardware
> but I haven't got round to that yet. (Also since it has utility outside of
> just testing qemu I'm not sure if it really belongs in the qemu repo.)

That does sound a little heavyweight.  Scripting gdb is also a possibility.

FWIW--and this is not particularly conducive to random insn
sequences--the approach taken when doing the AltiVec bits was to have
code that looked like:

  for each insn:
     for a suitable set of inputs:
        setup interesting registers (status control registers etc.)
        load inputs into registers
        execute
        record interesting post conditions in file.out

You'd get an output file from real hardware and an output file from the
simulator and then compare them, fixing differences as you go.  The
actual code included bits to compare the files as well as doing the
generation.

The output files can be somewhat large, but I'm sure clever engineering
could be applied to make them smaller.

Of course, the *real* problems are in undefined-behavior land. :)

-Nathan
Peter Maydell - Dec. 6, 2010, 5:07 p.m.
On 6 December 2010 16:57, Nathan Froyd <froydnj@codesourcery.com> wrote:
> FWIW--and this is not particularly conducive to random insn
> sequences--the approach taken when doing the AltiVec bits was to have
> code that looked like:
>
>  for each insn:
>     for a suitable set of inputs:
>        setup interesting registers (status control registers etc.)
>        load inputs into registers
>        execute
>        record interesting post conditions in file.out

Mmm. The trouble with that is it's more faff per instruction than I'd
like. The nice thing about random instruction sequences is you can
have a config file that says:
VQSHL_reg_a A1 1111 001 u 0 d sz:2 vn:4 vd:4 0100 n 0 m 1 vm:4
VQSHL_reg_b A1 1111 001 u 0 d sz:2 vn:3 0 vd:3 0 0100 n 1 m 1 vm:3 0

and that's all you need to test the VQSHL(reg) forms, and it's basically
the instruction format out of the ARM ARM. (In fact it ought to be a
single line but qemu doesn't correctly UNDEF on the Q==1, lsbit of
Vd/Vn/Vm!=0 case so I am avoiding generating it.)

> You'd get an output file from real hardware and an output file from the
> simulator and then compare them, fixing differences as you go.  The
> actual code included bits to compare the files as well as doing the
> generation.

You could certainly do the 'record/replay file' part; at the moment
what I have does about what I want, though :-).
(I really must get round to writing the README and sticking it
in a public git repo.)

> Of course, the *real* problems are in undefined-behavior land. :)

Heh. We don't even correctly handle "entirely well defined and should
UNDEF" land yet...

-- PMM
Peter Maydell - Dec. 8, 2010, noon
On 6 December 2010 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:
(regarding my random-instruction-sequence testing tool)
> (I really must get round to writing the README and sticking it
> in a public git repo.)

The curious can find it here:

http://git.linaro.org/gitweb?p=people/pmaydell/risu.git;a=summary

-- PMM

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 99464ab..0c8439a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -2870,16 +2870,18 @@  static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     VFP_DREG_N(rn, insn);
                 }
 
-                if (op == 15 && (rn == 15 || rn > 17)) {
+                if (op == 15 && (rn == 15 || ((rn & 0x1c) == 0x18))) {
                     /* Integer or single precision destination.  */
                     rd = VFP_SREG_D(insn);
                 } else {
                     VFP_DREG_D(rd, insn);
                 }
-
-                if (op == 15 && (rn == 16 || rn == 17)) {
-                    /* Integer source.  */
-                    rm = ((insn << 1) & 0x1e) | ((insn >> 5) & 1);
+                if (op == 15 && 
+                    (((rn & 0x1c) == 0x10) || ((rn & 0x14) == 0x14))) {
+                    /* VCVT from int is always from S reg regardless of dp bit.
+                     * VCVT with immediate frac_bits has same format as SREG_M
+                     */
+                    rm = VFP_SREG_M(insn);
                 } else {
                     VFP_DREG_M(rm, insn);
                 }
@@ -2891,6 +2893,9 @@  static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 } else {
                     rd = VFP_SREG_D(insn);
                 }
+                /* NB that we implicitly rely on the encoding for the frac_bits
+                 * in VCVT of fixed to float being the same as that of an SREG_M.
+                 */
                 rm = VFP_SREG_M(insn);
             }
 
@@ -3179,8 +3184,8 @@  static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 /* Write back the result.  */
                 if (op == 15 && (rn >= 8 && rn <= 11))
                     ; /* Comparison, do nothing.  */
-                else if (op == 15 && rn > 17)
-                    /* Integer result.  */
+                else if (op == 15 && dp && ((rn & 0x1c) == 0x18))
+                    /* VCVT double to int: always integer result. */
                     gen_mov_vreg_F0(0, rd);
                 else if (op == 15 && rn == 15)
                     /* conversion */