[PATCH] tests/tcg/hexagon: fix underspecifed asm constraints

Mukilan Thiyagarajan posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221228153657.10210-1-quic._5Fmthiyaga@quicinc.com
Maintainers: Taylor Simpson <tsimpson@quicinc.com>
There is a newer version of this series
tests/tcg/hexagon/mem_noshuf.c | 2 +-
tests/tcg/hexagon/misc.c       | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[PATCH] tests/tcg/hexagon: fix underspecifed asm constraints
Posted by Mukilan Thiyagarajan 1 year, 4 months ago
There are two test cases where the inline asm doesn't
have the correct constraints causing them to fail when
using certain clang versions/optimization levels.

In mem_noshuf.c, the register r7 is written to but
not specified in the clobber list.

In misc.c, the 'result' output needs the early clobber
modifier since the rest of the inputs are read after
assignment to the output register.

Signed-off-by: Mukilan Thiyagarajan <quic_mthiyaga@quicinc.com>
---
 tests/tcg/hexagon/mem_noshuf.c | 2 +-
 tests/tcg/hexagon/misc.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/hexagon/mem_noshuf.c b/tests/tcg/hexagon/mem_noshuf.c
index 0f4064e700..210b2f1208 100644
--- a/tests/tcg/hexagon/mem_noshuf.c
+++ b/tests/tcg/hexagon/mem_noshuf.c
@@ -144,7 +144,7 @@ static inline long long pred_ld_sd_pi(int pred, long long *p, long long *q,
                  "}:mem_noshuf\n"
                  : "=&r"(ret)
                  : "r"(p), "r"(q), "r"(x), "r"(y), "r"(pred)
-                 : "p0", "memory");
+                 : "r7", "p0", "memory");
     return ret;
 }
 
diff --git a/tests/tcg/hexagon/misc.c b/tests/tcg/hexagon/misc.c
index f0b1947fb3..9b1604d82f 100644
--- a/tests/tcg/hexagon/misc.c
+++ b/tests/tcg/hexagon/misc.c
@@ -189,7 +189,7 @@ static int L2_ploadrifnew_pi(void *p, int pred)
                "    p0 = cmp.eq(%1, #1)\n\t"
                "    if (!p0.new) %0 = memw(%2++#4)\n\t"
                "}\n\t"
-               : "=r"(result) : "r"(pred), "r"(p)
+               : "=&r"(result) : "r"(pred), "r"(p)
                : "p0");
   return result;
 }
-- 
2.17.1
RE: [PATCH] tests/tcg/hexagon: fix underspecifed asm constraints
Posted by Taylor Simpson 1 year, 4 months ago

> -----Original Message-----
> From: Mukilan Thiyagarajan (QUIC) <quic_mthiyaga@quicinc.com>
> Sent: Wednesday, December 28, 2022 9:37 AM
> To: qemu-devel@nongnu.org; Taylor Simpson <tsimpson@quicinc.com>
> Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; Mukilan Thiyagarajan (QUIC)
> <quic_mthiyaga@quicinc.com>
> Subject: [PATCH] tests/tcg/hexagon: fix underspecifed asm constraints
> 
> There are two test cases where the inline asm doesn't have the correct
> constraints causing them to fail when using certain clang
> versions/optimization levels.
> 
> In mem_noshuf.c, the register r7 is written to but not specified in the clobber
> list.
> 
> In misc.c, the 'result' output needs the early clobber modifier since the rest
> of the inputs are read after assignment to the output register.
> 
> Signed-off-by: Mukilan Thiyagarajan <quic_mthiyaga@quicinc.com>
> ---
>  tests/tcg/hexagon/mem_noshuf.c | 2 +-
>  tests/tcg/hexagon/misc.c       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/tcg/hexagon/misc.c b/tests/tcg/hexagon/misc.c index
> f0b1947fb3..9b1604d82f 100644
> --- a/tests/tcg/hexagon/misc.c
> +++ b/tests/tcg/hexagon/misc.c
> @@ -189,7 +189,7 @@ static int L2_ploadrifnew_pi(void *p, int pred)
>                 "    p0 = cmp.eq(%1, #1)\n\t"
>                 "    if (!p0.new) %0 = memw(%2++#4)\n\t"
>                 "}\n\t"
> -               : "=r"(result) : "r"(pred), "r"(p)
> +               : "=&r"(result) : "r"(pred), "r"(p)

Good catch.  However, the "p" argument is also modified, so we need to move it to the outputs list and use "+r"(p).  This will also require swapping %1 and %2 in the body.

Otherwise
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
RE: [PATCH] tests/tcg/hexagon: fix underspecifed asm constraints
Posted by Mukilan Thiyagarajan (QUIC) 1 year, 4 months ago
Thanks for the review, Taylor!

> Good catch.  However, the "p" argument is also modified, so we need to move it to
> the outputs list and use "+r"(p).  This will also require swapping %1 and %2 in the body.

Ah, good point. I didn't account for memw(%2++#4) incrementing register %2.
I've made the suggested changes in v2 of the patch:

https://patchew.org/QEMU/20221229081836.12130-1-quic._5Fmthiyaga@quicinc.com/

Regards,
Mukilan

-----Original Message-----
From: Taylor Simpson <tsimpson@quicinc.com> 
Sent: Wednesday, December 28, 2022 11:07 PM
To: Mukilan Thiyagarajan (QUIC) <quic_mthiyaga@quicinc.com>; qemu-devel@nongnu.org
Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>
Subject: RE: [PATCH] tests/tcg/hexagon: fix underspecifed asm constraints



> -----Original Message-----
> From: Mukilan Thiyagarajan (QUIC) <quic_mthiyaga@quicinc.com>
> Sent: Wednesday, December 28, 2022 9:37 AM
> To: qemu-devel@nongnu.org; Taylor Simpson <tsimpson@quicinc.com>
> Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; Mukilan Thiyagarajan (QUIC)
> <quic_mthiyaga@quicinc.com>
> Subject: [PATCH] tests/tcg/hexagon: fix underspecifed asm constraints
> 
> There are two test cases where the inline asm doesn't have the correct
> constraints causing them to fail when using certain clang
> versions/optimization levels.
> 
> In mem_noshuf.c, the register r7 is written to but not specified in the clobber
> list.
> 
> In misc.c, the 'result' output needs the early clobber modifier since the rest
> of the inputs are read after assignment to the output register.
> 
> Signed-off-by: Mukilan Thiyagarajan <quic_mthiyaga@quicinc.com>
> ---
>  tests/tcg/hexagon/mem_noshuf.c | 2 +-
>  tests/tcg/hexagon/misc.c       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/tcg/hexagon/misc.c b/tests/tcg/hexagon/misc.c index
> f0b1947fb3..9b1604d82f 100644
> --- a/tests/tcg/hexagon/misc.c
> +++ b/tests/tcg/hexagon/misc.c
> @@ -189,7 +189,7 @@ static int L2_ploadrifnew_pi(void *p, int pred)
>                 "    p0 = cmp.eq(%1, #1)\n\t"
>                 "    if (!p0.new) %0 = memw(%2++#4)\n\t"
>                 "}\n\t"
> -               : "=r"(result) : "r"(pred), "r"(p)
> +               : "=&r"(result) : "r"(pred), "r"(p)

Good catch.  However, the "p" argument is also modified, so we need to move it to the outputs list and use "+r"(p).  This will also require swapping %1 and %2 in the body.

Otherwise
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>