On Thu, 2023-03-16 at 18:50 +0100, Nina Schoetterl-Glausch wrote:
> 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 <i...@linux.ibm.com>
>
> Reviewed-by: Nina Schoetterl-Glausch <n...@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?
I was worried that mem would become inaccessible from the asm block,
but apparently it still works if I make mem static.
> > > +#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.
Ok, will do.
> > > + "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) \
OT: I just realized I needed to use "a" instead of "r" here and in a
few other places.
> > > + , [target] "=&r"
> > > (target) \
> > > + , [reg_val] "+&r"
> > > (reg_val) \
> > > + : : "cc",
> > > "memory"); \
> > > + reg =
> > > reg_val;
> > > \
>
> What is the point of this assignment?
This is paranoia induced by the conservative reading of [1].
In this case I think the compiler is free to clobber reg_val during the
assignment to *cc.
[1] https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html
>
> > > + *cc = (cc_val >> 28) &
> > > 3; \
> > > +
> > > \
> > > + return
> > > reg_val; \
... so I should rather be returning reg here.
> > > + }
> > > +
> > > +#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; \
Same here.
> > > + }
> > > +
> > > +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?
Right, that's also a leftover. I'll clean this up.
[...]
