From: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>,
Richard Henderson <richard.henderson@linaro.org>,
David Hildenbrand <david@redhat.com>,
Thomas Huth <thuth@redhat.com>
Cc: qemu-s390x@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c
Date: Thu, 16 Mar 2023 18:50:00 +0100 [thread overview]
Message-ID: <5289dc2f8cc7c1b2ee3b6693b54b3b98746b4d0b.camel@linux.ibm.com> (raw)
In-Reply-To: <20230315001117.337128-3-iii@linux.ibm.com>
On Wed, 2023-03-15 at 01:11 +0100, Ilya Leoshkevich wrote:
> > Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
> > as targets.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Some comments below.
> > ---
> > tests/tcg/s390x/Makefile.target | 1 +
> > tests/tcg/s390x/ex-relative-long.c | 159 +++++++++++++++++++++++++++++
> > 2 files changed, 160 insertions(+)
> > create mode 100644 tests/tcg/s390x/ex-relative-long.c
> >
> > diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> > index cf93b966862..90bc48227db 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -29,6 +29,7 @@ TESTS+=clst
> > TESTS+=long-double
> > TESTS+=cdsg
> > TESTS+=chrl
> > +TESTS+=ex-relative-long
> >
> > cdsg: CFLAGS+=-pthread
> > cdsg: LDFLAGS+=-pthread
> > diff --git a/tests/tcg/s390x/ex-relative-long.c b/tests/tcg/s390x/ex-relative-long.c
> > new file mode 100644
> > index 00000000000..4caa8c1b962
> > --- /dev/null
> > +++ b/tests/tcg/s390x/ex-relative-long.c
> > @@ -0,0 +1,159 @@
> > +/* Check EXECUTE with relative long instructions as targets. */
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +struct test {
> > + const char *name;
> > + long (*func)(long reg, long *cc);
> > + long exp_reg;
> > + long exp_mem;
> > + long exp_cc;
> > +};
> > +
> > +/*
> > + * Each test sets the MEM_IDXth element of the mem array to MEM and uses a
> > + * single relative long instruction on it. The other elements remain zero.
> > + * This is in order to prevent stumbling upon MEM in random memory in case
> > + * there is an off-by-a-small-value bug.
> > + *
> > + * Note that while gcc supports the ZL constraint for relative long operands,
> > + * clang doesn't, so the assembly code accesses mem[MEM_IDX] using MEM_ASM.
> > + */
> > +long mem[0x1000];
This could be static, no?
> > +#define MEM_IDX 0x800
> > +#define MEM_ASM "mem+0x800*8"
> > +
> > +/* Initial %r2 value. */
> > +#define REG 0x1234567887654321
> > +
> > +/* Initial mem[MEM_IDX] value. */
> > +#define MEM 0xfedcba9889abcdef
> > +
> > +/* Initial cc value. */
> > +#define CC 0
> > +
> > +/* Relative long instructions and their expected effects. */
> > +#define FOR_EACH_INSN(F) \
You could define some macros and then calculate a bunch of values in the table, i.e.
#define SL(v) ((long)(v))
#define UL(v) ((unsigned long)(v))
#define SI(v, i) ((int)(v >> ((1 - i) * 32)))
#define UI(v, i) ((unsigned int)(v >> ((1 - i) * 32)))
#define SH(v, i) ((short)(v >> ((3 - i) * 16)))
#define UH(v, i) ((unsigned short)(v >> ((3 - i) * 16)))
#define CMP(f, s) ((f) == (s) ? 0 : ((f) < (s) ? 1 : 2 ))
F(cgfrl, REG, MEM, CMP(SL(REG), SI(MEM, 0))
But everything checks out, so no need.
> > + F(cgfrl, REG, MEM, 2) \
> > + F(cghrl, REG, MEM, 2) \
> > + F(cgrl, REG, MEM, 2) \
> > + F(chrl, REG, MEM, 1) \
> > + F(clgfrl, REG, MEM, 2) \
> > + F(clghrl, REG, MEM, 2) \
> > + F(clgrl, REG, MEM, 1) \
> > + F(clhrl, REG, MEM, 2) \
> > + F(clrl, REG, MEM, 1) \
> > + F(crl, REG, MEM, 1) \
> > + F(larl, (long)&mem[MEM_IDX], MEM, CC) \
> > + F(lgfrl, 0xfffffffffedcba98, MEM, CC) \
> > + F(lghrl, 0xfffffffffffffedc, MEM, CC) \
> > + F(lgrl, MEM, MEM, CC) \
> > + F(lhrl, 0x12345678fffffedc, MEM, CC) \
> > + F(llghrl, 0x000000000000fedc, MEM, CC) \
> > + F(llhrl, 0x123456780000fedc, MEM, CC) \
> > + F(lrl, 0x12345678fedcba98, MEM, CC) \
> > + F(stgrl, REG, REG, CC) \
> > + F(sthrl, REG, 0x4321ba9889abcdef, CC) \
> > + F(strl, REG, 0x8765432189abcdef, CC)
> > +
> > +/* Test functions. */
> > +#define DEFINE_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + static long test_ex_ ## insn(long reg, long *cc) \
> > + { \
> > + register long reg_val asm("r2"); \
> > + long cc_val, mask, target; \
> > + \
> > + reg_val = reg; \
> > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
This confused me for a bit because it's not about cc_val at all.
Maybe do "cr %%r0,%%r0\n" instead?
Could also push it down before the ex, since that's where the change we care about
takes place.
> > + "lghi %[mask],0x20\n" /* make target use %r2 */ \
> > + "larl %[target],0f\n" \
> > + "ex %[mask],0(%[target])\n" \
> > + "jg 1f\n" \
> > + "0: " #insn " %%r0," MEM_ASM "\n" \
> > + "1: ipm %[cc_val]\n" \
> > + : [cc_val] "=&r" (cc_val) \
> > + , [mask] "=&r" (mask) \
> > + , [target] "=&r" (target) \
> > + , [reg_val] "+&r" (reg_val) \
> > + : : "cc", "memory"); \
> > + reg = reg_val; \
What is the point of this assignment?
> > + *cc = (cc_val >> 28) & 3; \
> > + \
> > + return reg_val; \
> > + }
> > +
> > +#define DEFINE_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + static long test_exrl_ ## insn(long reg, long *cc) \
> > + { \
> > + register long reg_val asm("r2"); \
> > + long cc_val, mask; \
> > + \
> > + reg_val = reg; \
> > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
> > + "lghi %[mask],0x20\n" /* make target use %r2 */ \
> > + "exrl %[mask],0f\n" \
> > + "jg 1f\n" \
> > + "0: " #insn " %%r0," MEM_ASM "\n" \
> > + "1: ipm %[cc_val]\n" \
> > + : [cc_val] "=&r" (cc_val) \
> > + , [mask] "=&r" (mask) \
> > + , [reg_val] "+&r" (reg_val) \
> > + : : "cc", "memory"); \
> > + reg = reg_val; \
> > + *cc = (cc_val >> 28) & 3; \
> > + \
> > + return reg_val; \
> > + }
> > +
> > +FOR_EACH_INSN(DEFINE_EX_TEST)
> > +FOR_EACH_INSN(DEFINE_EXRL_TEST)
> > +
> > +/* Test definitions. */
> > +#define REGISTER_EX_EXRL_TEST(ex_insn, insn, _exp_reg, _exp_mem, _exp_cc) \
> > + { \
> > + .name = #ex_insn " " #insn, \
> > + .func = test_ ## ex_insn ## _ ## insn, \
> > + .exp_reg = (long)(_exp_reg), \
> > + .exp_mem = (long)(_exp_mem), \
> > + .exp_cc = (long)(_exp_cc), \
You don't need the casts, do you?
> > + },
> > +
> > +#define REGISTER_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + REGISTER_EX_EXRL_TEST(ex, insn, exp_reg, exp_mem, exp_cc)
> > +
> > +#define REGISTER_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + REGISTER_EX_EXRL_TEST(exrl, insn, exp_reg, exp_mem, exp_cc)
> > +
> > +static const struct test tests[] = {
> > + FOR_EACH_INSN(REGISTER_EX_TEST)
> > + FOR_EACH_INSN(REGISTER_EXRL_TEST)
> > +};
> > +
> > +/* Loop over all tests and run them. */
> > +int main(void)
> > +{
> > + const struct test *test;
> > + int ret = EXIT_SUCCESS;
> > + long reg, cc;
> > + size_t i;
> > +
> > + for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
> > + test = &tests[i];
> > + mem[MEM_IDX] = MEM;
> > + cc = -1;
> > + reg = test->func(REG, &cc);
> > +#define ASSERT_EQ(expected, actual) do { \
> > + if (expected != actual) { \
> > + fprintf(stderr, "%s: " #expected " (0x%lx) != " #actual " (0x%lx)\n", \
> > + test->name, expected, actual); \
> > + ret = EXIT_FAILURE; \
> > + } \
> > +} while (0)
> > + ASSERT_EQ(test->exp_reg, reg);
> > + ASSERT_EQ(test->exp_mem, mem[MEM_IDX]);
> > + ASSERT_EQ(test->exp_cc, cc);
> > +#undef ASSERT_EQ
> > + }
> > +
> > + return ret;
> > +}
next prev parent reply other threads:[~2023-03-16 17:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 0:11 [PATCH v2 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
2023-03-15 8:58 ` David Hildenbrand
2023-03-15 14:33 ` Richard Henderson
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
2023-03-15 14:33 ` Richard Henderson
2023-03-16 17:50 ` Nina Schoetterl-Glausch [this message]
2023-03-16 20:55 ` Ilya Leoshkevich
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=5289dc2f8cc7c1b2ee3b6693b54b3b98746b4d0b.camel@linux.ibm.com \
--to=nsg@linux.ibm.com \
--cc=david@redhat.com \
--cc=iii@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=thuth@redhat.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).