RE: [PATCH v7 12/13] target/hexagon: import additional tests

[Taylor Simpson <tsimpson@quicinc.com>]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Friday, December 17, 2021 2:01 AM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; babush@rev.ng; nizzo@rev.ng;
> richard.henderson@linaro.org
> Subject: [PATCH v7 12/13] target/hexagon: import additional tests
> 
> 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/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index 8b07a28166..6f1c0830aa 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -24,7 +24,7 @@ CFLAGS += -fno-unroll-loops
> HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon
>  VPATH += $(HEX_SRC)
> 
> -first: $(HEX_SRC)/first.S
> +%: $(HEX_SRC)/%.S $(HEX_SRC)/crt.S
>         $(CC) -static -mv67 -nostdlib $^ -o $@
> 
>  HEX_TESTS = first
> @@ -42,4 +42,32 @@ HEX_TESTS += atomics
>  HEX_TESTS += fpstuff
>  HEX_TESTS += overflow
> 
> +HEX_TESTS += test_abs
> +HEX_TESTS += test_bitcnt
> +HEX_TESTS += test_bitsplit
> +HEX_TESTS += test_clobber

You forgot to add test_call to this list.

> +HEX_TESTS += test_cmp
> +HEX_TESTS += test_cmpy


> diff --git a/tests/tcg/hexagon/test_call.S b/tests/tcg/hexagon/test_call.S
> new file mode 100644 index 0000000000..fe59a8f006
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_call.S
> @@ -0,0 +1,60 @@
> +/* 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.
> + */
> +    {
> +        call write
> +    }
> +    {
> +        jump pass
> +    }
> +    {
> +        r31:30=deallocframe(r30):raw
> +    }

The jump to pass will never return, so the deallocframe is unreachable.

> +.Lfunc_end1:
> +.Ltmp1:
> +    .size    _start, .Ltmp1-_start
> +write:
> +    {
> +        r0=##dummy_buffer
> +    }
> +    {
> +        r1=#256
> +    }

Were you intending to make a system call here?  If so, the sequence would be (see first.S for #define's)
	r6 = #SYS_write
	r0 = #FD_STDOUT
	r1 = ##dummy_buffer
	r2 = #33
	trap0(#1)

> +    {
> +        jumpr r31
> +    }


> diff --git a/tests/tcg/hexagon/test_cmpy.S
> b/tests/tcg/hexagon/test_cmpy.S new file mode 100644 index
> 0000000000..0b3dfb95de
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_cmpy.S
> @@ -0,0 +1,31 @@
> +/* Purpose: test example, verify the soundness of the cmpy operation
> + *
> + *  3j+5 * 2j+4 = 22j+14
> + *
> + * the complex multiply between 0x00030005 and 0x00020004 is
> +0x000000160000000e  */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r0=#196613
> +        r1=#131076
> +    }
> +    {
> +        r3:2=cmpy(r0, r1):sat
> +    }

You should also check the overflow bit in USR from the :sat.
Also, have one set of inputs that overflows and one that doesn't in order to verify the proper USR behavior.

> +    {
> +        p0 = cmp.eq(r2, #14); if (p0.new) jump:t test2
> +        jump fail
> +    }
> +
> +test2:
> +    {
> +        p0 = cmp.eq(r3, #22); if (p0.new) 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..70886d9483
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_dotnew.S
> @@ -0,0 +1,36 @@
> +/* Purpose: test the .new operator while performing memory stores. */
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r0=#1
> +        memw(sp+#0)=r0.new
> +    }

You should allocframe before writing to the stack.

> 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 */

This test is named test_jmp.S, but you are testing add?

diff --git a/tests/tcg/hexagon/test_overflow.S b/tests/tcg/hexagon/test_overflow.S
new file mode 100644
index 0000000000..9e2a235616
--- /dev/null
+++ b/tests/tcg/hexagon/test_overflow.S
@@ -0,0 +1,63 @@
+// Purpose: test example, verify the soundness of the overflow bit // 
+// a right shift with negative amount should make r0 saturate, setting 
+the // overflow bit
+
+    .text
+    .globl _start
+
+_start:
+    {
+        call init
+    }
+    {
+        r0=#0x10000000
+        r1=#-2
+    }

You should clear bit 0 in USR before doing the asr(r0,r1):sat.

Also, do this for the other places in this review that mention USR.

+    {
+        r2=asr(r0, r1):sat
+    }
+    {
+        r4=USR
+    }
+    {
+        r5=and(r4,#1)
+    }

> 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
> +    }

Need allocframe first

> +    {
> +        r3=memw(sp+#0)
> +        r0=add(r2,r3)
> +    }
> +    {
> +        p0 = cmp.eq(r0, #10); if (p0.new) jump:t pass
> +        jump fail
> +    }

Also check that r3 == 4


> diff --git a/tests/tcg/hexagon/test_vcmpb.S
> b/tests/tcg/hexagon/test_vcmpb.S new file mode 100644 index
> 0000000000..3c6700a63a
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_vcmpb.S
> @@ -0,0 +1,32 @@
> +/* Purpose: test example, verify the soundness of the vector compare
> +bytes
> + * operation
> + *
> + * Vector word comparison between 0x1234567887654321 and
> +0x1234567800000000
> + * should result in 0x11110000

Comment is incorrect - should be Vector byte comparison.

> diff --git a/tests/tcg/hexagon/test_vcmpw.S
> b/tests/tcg/hexagon/test_vcmpw.S new file mode 100644 index
> 0000000000..112f08c92f
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_vcmpw.S
> @@ -0,0 +1,29 @@
> +/* Purpose: test example, verify the soundness of the vector compare
> +words
> + * operation
> + *
> + * Vector word comparison between 0x1234567887654321 and
> +0x1234567800000000
> + * should result in 0x11110000

That's the expected result in binary, not hex.

> + */
> +
> +    .text
> +    .globl _start
> +
> +_start:
> +    {
> +        call init
> +    }
> +    {
> +        r0=#305419896
> +        r1=#-2023406815
> +    }
> +    {
> +        r2=#305419896
> +        r3=#0
> +    }
> +    {
> +        p2=vcmpw.eq(r1:0, r3:2)
> +    }
> +    {
> +        if (p2) jump:t pass

Check for the actual expected value, not just LSB.

Also, this doesn't match the comment above.  You're actually comparing
    0x8765432112345678
    0x0000000012345678
So, p2 will have 0xf, not 0xf0.

In other words, the assignments to r0/r1 and r2/r3 are swapped.

Go ahead and swap them and check that p2 gets 0xf0.
 

> diff --git a/tests/tcg/hexagon/test_vcmpy.S
> b/tests/tcg/hexagon/test_vcmpy.S new file mode 100644 index
> 0000000000..df379f9186
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_vcmpy.S
> +    {
> +        r5:4=vcmpyr(r1:0, r3:2):sat
> +        r7:6=vcmpyi(r1:0, r3:2):sat
> +    }

Create some inputs that will saturate and check for overflow in USR.

> diff --git a/tests/tcg/hexagon/test_vspliceb.S
> b/tests/tcg/hexagon/test_vspliceb.S
> new file mode 100644
> index 0000000000..bae2a9c163
> --- /dev/null
> +++ b/tests/tcg/hexagon/test_vspliceb.S
> @@ -0,0 +1,33 @@
> +/* Purpose: test example, verify the soundness of the vspliceb
> +operation
> + * the operation is a binary splice of two 64bit operators
> + *
> + *  vspliceb(0xffffffffffffffff,0x0000000000000000,5) =
> +0x000000000000001f  */

The comment is incorrect.  The result is 0x000000ffffffffff.

The test is correct, though.