All of lore.kernel.org
 help / color / mirror / Atom feed
From: Taylor Simpson <taylor.qemu@gmail.com>
To: Alessandro Di Federico <ale.qemu@rev.ng>
Cc: Alessandro Di Federico <ale@rev.ng>,
	bcain@quicinc.com, richard.henderson@linaro.org,
	qemu-devel@nongnu.org, babush@rev.ng, tsimpson@quicinc.com,
	nizzo@rev.ng, philmd@redhat.com
Subject: Re: [PATCH v5 13/14] target/hexagon: import additional tests
Date: Fri, 25 Jun 2021 18:56:57 -0500	[thread overview]
Message-ID: <CANji28b-AWkJ0C-Co5dYop2ykKgjjR96bw6e+MoaYtukUXKWEg@mail.gmail.com> (raw)
In-Reply-To: <20210619093713.1845446-14-ale.qemu@rev.ng>

[-- Attachment #1: Type: text/plain, Size: 10996 bytes --]

On Sat, Jun 19, 2021 at 4:49 AM Alessandro Di Federico via <
qemu-devel@nongnu.org> wrote:

> From: Niccolò Izzo <nizzo@rev.ng>
>
> Signed-off-by: Alessandro Di Federico <ale@rev.ng>
> Signed-off-by: Niccolò Izzo <nizzo@rev.ng>
> ---
>


> diff --git a/tests/tcg/hexagon/crt.S b/tests/tcg/hexagon/crt.S
> new file mode 100644
> index 0000000000..2c10577470
> --- /dev/null
> +++ b/tests/tcg/hexagon/crt.S
> @@ -0,0 +1,28 @@
> +#define SYS_exit_group           94
> +
> +    .text
> +    .globl init
> +init:
> +    {
> +        allocframe(r29,#0):raw
> +    }
> +    {
> +        r0=#256
> +    }
> +    {
> +        dealloc_return
> +    }
>

You don't need to alloc/dealloc the frame, just a single packet { r0 =
#256; jumpr r31 }

Then again, why is this function needed at all?


+
> +    .space 240
>

Why is this space needed?

diff --git a/tests/tcg/hexagon/test_andp.S b/tests/tcg/hexagon/test_andp.S
> new file mode 100644
> index 0000000000..3c4aa8b2ae
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_andp.S
> @@ -0,0 +1,23 @@
> +/* Purpose: test a multiple predicate AND combination */
>

This is already tested in misc.c

+
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        r1+=sub(r2,r3)
>

Remove this - r1, r2, and r3 are uninitialized.  Then, you overwrite r1 in
the next packet.


> +        call init
> +    }
> +    {
> +        r0=#0
> +        r1=#1
> +    }
> +    {
> +        p0=cmp.gt(r0,r1)
> +        p0=cmp.gt(r0,r1)
> +        p0=cmp.gt(r1,r0)
> +    }
> +    {
> +        if (!p0) jump:t pass
> +        jump fail
> +    }
>
> diff --git a/tests/tcg/hexagon/test_call.S b/tests/tcg/hexagon/test_call.S
> new file mode 100644
> index 0000000000..53a2450522
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_call.S
> @@ -0,0 +1,63 @@
> +/* Purpose: test function calls and duplex instructions.
> + * The string "Hello there, I'm a test string!" with the first letter
> replaced
> + * with a capital L should be printed out.
> + */
> +
> +    .text
> +    .globl    test
> +test:
> +    {
> +        jumpr r31
> +        memb(r0+#0)=#76
> +    }
> +.Lfunc_end0:
> +.Ltmp0:
> +    .size    test, .Ltmp0-test
> +
> +    .globl    _start
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        call test
> +        r0=##dummy_buffer
> +        allocframe(#0)
> +    }
> +    {
> +        call write
> +    }
> +    {
> +        jump pass
> +    }
> +    {
> +        r31:30=deallocframe(r30):raw
> +    }
> +.Lfunc_end1:
> +.Ltmp1:
> +    .size    _start, .Ltmp1-_start
> +write:
> +    {
> +        r2=##dummy_buffer
> +    }
> +    { r0=r2; }
>

Assign to r0 directly


> +    {
> +        r2=#256
> +    }
> +    { r1=r2; }
>

Assign to r1 directly

+    { trap0(#7); }
>

This doesn't print anything - the syscall number goes in r6.  Is the intent
to invoke SYS_write?  If so, look at first.S to see how this is done.

+    {
> +        jumpr r31
> +    }
> +.Lfunc_end2:
> +.Ltmp2:
> +    .size    write, .Ltmp2-write
> +
> +    .type    dummy_buffer,@object
> +    .data
> +    .globl    dummy_buffer
> +    .p2align    3
> +dummy_buffer:
> +    .string    "Hello there, I'm a test string!\n"
> +    .space 223
> +    .size    dummy_buffer, 256
>



> diff --git a/tests/tcg/hexagon/test_djump.S
> b/tests/tcg/hexagon/test_djump.S
> new file mode 100644
> index 0000000000..dbad7eb0a1
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_djump.S
> @@ -0,0 +1,24 @@
> +/* Purpose: show dual jumps actually work. This program features a packet
> where
> + * two jumps should (in theory) be performed if !P0. However, we correctly
> + * handle the situation by performing only the first one and ignoring the
> second
> + * one. This can be verified by checking that the CPU dump contains
> 0xDEADBEEF
> + * in R2.
> + */
>

How does 0xDEADBEEF get into r2?

Do we need this test given that every other test relies on this idiom to
check pass/fail?

+
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r1 = #255;
> +    }
> +    {
> +        p0 = r1;
> +    }
> +    {
> +        if (p0) jump:t pass
> +        jump fail
> +    }
> diff --git a/tests/tcg/hexagon/test_dotnew.S
> b/tests/tcg/hexagon/test_dotnew.S
> new file mode 100644
> index 0000000000..3897c6bc96
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_dotnew.S
> @@ -0,0 +1,39 @@
> +/* Purpose: test the .new operator while performing memory stores.
> + * In the final CPU dump R0 should contain 3, R1 should contain 2 and R2
> should
> + * contain 1.
> + */
>

CPU dump sounds like something you have locally.  The check-tcg tests
should not rely on this.

+    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r0=#1
> +        memw(sp+#0)=r0.new
> +    }
>

You haven't set up the stack, so you shouldn't use sp.  Even if the stack
were set up, you should allocframe first.

+    {
> +        r1=#2
> +        memw(sp+#4)=r1.new
> +    }
> +    {
> +        r2=#3
> +        memw(sp+#8)=r2.new
> +    }
> +    {
> +        r0=memw(sp+#8)
> +    }
> +    {
> +        r1=memw(sp+#4)
> +    }
> +    {
> +        r2=memw(sp+#0)
> +    }
> +    {
> +        r3=mpyi(r1,r2)
> +    }
> +    {
> +        p0 = cmp.eq(r3, #2); if (p0.new) jump:t pass
> +        jump fail
> +    }
> diff --git a/tests/tcg/hexagon/test_dstore.S
> b/tests/tcg/hexagon/test_dstore.S
> new file mode 100644
> index 0000000000..62c4301eb1
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_dstore.S
> @@ -0,0 +1,29 @@
> +/* Purpose: test dual stores correctness.
> + * In this example the values 1 and 2 are both written on the top of the
> stack
> + * in a single packet.
> + * The value is then read back in R3, which should contain only the
> latest value
> + * written (2).
> + */
>

 This is already tested in dual_stores.c

+
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r0=#1
> +        r1=#2
> +    }
> +    {
> +        memw(sp+#0)=r0
> +        memw(sp+#0)=r1
> +    }
>

Stack setup ...

+    {
> +        r3=memw(sp+#0)
> +    }
> +    {
> +        p0 = cmp.eq(r3, #2); if (p0.new) jump:t pass
> +        jump fail
> +    }
> diff --git a/tests/tcg/hexagon/test_ext.S b/tests/tcg/hexagon/test_ext.S
> new file mode 100644
> index 0000000000..0f6e21593a
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_ext.S
> @@ -0,0 +1,18 @@
> +/* Purpose: test immediate extender instructions.
> + * In the CPU dump R0 should contain 0xDEADBEEF.
>

The CPU dump comment doesn't apply


> + */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r2=##-559038737
> +    }
> +    {
> +        p0 = cmp.eq(r2, ##-559038737); if (p0.new) jump:t pass
> +        jump fail
> +    }
>


> +++ b/tests/tcg/hexagon/test_hello.S
> @@ -0,0 +1,21 @@
> +/* Purpose: simple hello world program. */
>

This is already tested in first.S

+
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    { r0=#4; }
> +    {
> +        r1=##.L.str
> +    }
> +    { trap0(#0); }
> +    {
> +        jump pass
> +    }
> +
> +.L.str:
> +    .string    "Hello world!\n"
> +    .size    .L.str, 14
>


> diff --git a/tests/tcg/hexagon/test_hwloops.S
> b/tests/tcg/hexagon/test_hwloops.S
> new file mode 100644
> index 0000000000..8337083d8e
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_hwloops.S
> @@ -0,0 +1,25 @@
> +/* Purpose: simple C Program to test hardware loops.
> + * It should print numbersfrom 0 to 9.
>

This doesn't actually print anything.


> + */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        loop0(.LBB0_1,#10)
> +        r2=#0
> +    }
> +.Ltmp0:
> +.LBB0_1:
> +    {
> +        r2=add(r2,#1)
> +        nop
> +    }:endloop0
> +    {
> +        p0 = cmp.eq(r2, #10); if (p0.new) jump:t pass
> +        jump fail
> +    }
>


> diff --git a/tests/tcg/hexagon/test_jmp.S b/tests/tcg/hexagon/test_jmp.S
> new file mode 100644
> index 0000000000..9bf6ea34e5
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_jmp.S
> @@ -0,0 +1,25 @@
> +/* Purpose: test example, verify the soundness of the add operation */
>

What value does this test have beyond the others in this set?

+
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r1=#0
> +        r2=#0
> +    }
> +    {
> +        r3=add(r1,r2)
> +    }
> +    {
> +        p0 = cmp.eq(r3, #0)
> +    }
> +    {
> +        if (p0) jump:t pass
> +    }
> +    {
> +        jump fail
> +    }
>

diff --git a/tests/tcg/hexagon/test_packet.S
b/tests/tcg/hexagon/test_packet.S

> new file mode 100644
> index 0000000000..d26e284be9
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_packet.S
> @@ -0,0 +1,26 @@
> +/* Purpose: test that writes of a register in a packet are performed only
> after
> + * that packet has finished its execution.
> + */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r2=#4
> +        r3=#6
> +    }
> +    {
> +        memw(sp+#0)=r2
>

Stack problem

+    }
> +    {
> +        r3=memw(sp+#0)
> +        r0=add(r2,r3)
> +    }
> +    {
> +        p0 = cmp.eq(r0, #10); if (p0.new) jump:t pass
> +        jump fail
> +    }
> diff --git a/tests/tcg/hexagon/test_reorder.S
> b/tests/tcg/hexagon/test_reorder.S
> new file mode 100644
> index 0000000000..508d5302f9
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_reorder.S
> @@ -0,0 +1,31 @@
> +/* Purpose: demonstrate handling of .new uses appearing before the
> associated
> + * definition.
> + * Here we perform a jump that skips the code resetting R2 from
> 0xDEADBEEF to 0,
> + * only if P0.new is true, but P0 is assigned to 1 (R4) in the next
> instruction
> + * in the packet.
> + * A successful run of the program will show R2 retaining the 0xDEADBEEF
> value
> + * in the CPU dump.
>

CPU dump ...

+ */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    { r2=#-559038737 }
> +    { r4=#1 }
> +    {
> +        if (p0.new) jump:nt skip
> +        p0=r4;
> +    }
> +
> +fallthrough:
> +    { r2=#0 }
> +
> +skip:
> +    {
> +        p0 = cmp.eq(r2, #-559038737); if (p0.new) jump:t pass
> +        jump fail
> +    }
>

Each of these are very small, so I recommend putting them into misc.c or
combine all the assembly into a small number of executables.

[-- Attachment #2: Type: text/html, Size: 16999 bytes --]

  reply	other threads:[~2021-06-25 23:58 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-19  9:36 [PATCH v5 00/14] target/hexagon: introduce idef-parser Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 01/14] tcg: expose TCGCond manipulation routines Alessandro Di Federico via
2021-06-19 13:51   ` Richard Henderson
2021-06-19  9:37 ` [PATCH v5 02/14] target/hexagon: update MAINTAINERS for idef-parser Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 03/14] target/hexagon: import README " Alessandro Di Federico via
2021-06-23 15:46   ` Taylor Simpson
2021-06-24 13:51     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 04/14] target/hexagon: make slot number an unsigned Alessandro Di Federico via
2021-06-23 15:58   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 05/14] target/hexagon: make helper functions non-static Alessandro Di Federico via
2021-06-23 18:29   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 06/14] target/hexagon: introduce new helper functions Alessandro Di Federico via
2021-06-23 12:05   ` Taylor Simpson
2021-06-23 18:49   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 07/14] target/hexagon: expose next PC in DisasContext Alessandro Di Federico via
2021-06-23 18:54   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 08/14] target/hexagon: prepare input for the idef-parser Alessandro Di Federico via
2021-06-23 19:37   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 09/14] target/hexagon: import lexer for idef-parser Alessandro Di Federico via
2021-06-23 20:05   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 10/14] target/hexagon: import parser " Alessandro Di Federico via
2021-06-22 22:35   ` Taylor Simpson
2021-06-24  3:55   ` Taylor Simpson
2021-06-29 14:26     ` Alessandro Di Federico via
2021-06-30 16:51     ` Paolo Montesel
2021-07-05 16:47     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 11/14] target/hexagon: call idef-parser functions Alessandro Di Federico via
2021-06-25 22:00   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 12/14] target/hexagon: remove unused macros and functions Alessandro Di Federico via
2021-06-25 22:02   ` Taylor Simpson
2021-06-19  9:37 ` [PATCH v5 13/14] target/hexagon: import additional tests Alessandro Di Federico via
2021-06-25 23:56   ` Taylor Simpson [this message]
2021-06-28 22:39     ` Taylor Simpson
2021-07-05 16:50     ` Alessandro Di Federico via
2021-06-19  9:37 ` [PATCH v5 14/14] gitlab-ci: do not use qemu-project Docker registry Alessandro Di Federico via
2021-06-29 14:26   ` Alessandro Di Federico via
2021-06-29 14:37   ` Daniel P. Berrangé
2021-07-08 16:00     ` Alessandro Di Federico via

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANji28b-AWkJ0C-Co5dYop2ykKgjjR96bw6e+MoaYtukUXKWEg@mail.gmail.com \
    --to=taylor.qemu@gmail.com \
    --cc=ale.qemu@rev.ng \
    --cc=ale@rev.ng \
    --cc=babush@rev.ng \
    --cc=bcain@quicinc.com \
    --cc=nizzo@rev.ng \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=tsimpson@quicinc.com \
    --subject='Re: [PATCH v5 13/14] target/hexagon: import additional tests' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.