Re: [PATCH v7 3/4] tests/tcg/s390x: Tests for Miscellaneous-Instruction-Extensions Facility 3

[Thomas Huth]

On 24/02/2022 00.43, Richard Henderson wrote:
> On 2/23/22 12:31, David Miller wrote:
> > +#define F_EPI "stg %%r0, %[res] " : [res] "+m" (res) : : "r0", "r2", "r3"
> > +
> > +#define F_PRO    asm ( \
> > +    "llihf %%r0,801\n" \
> > +    "lg %%r2, %[a]\n"  \
> > +    "lg %%r3, %[b] "   \
> > +    : : [a] "m" (a),   \
> > +        [b] "m" (b)    \
> > +    : "r2", "r3")
> > +
> > +#define FbinOp(S, ASM) uint64_t S(uint64_t a, uint64_t b) \
> > +{ uint64_t res = 0; F_PRO; ASM; return res; }
> > +
> > +/* AND WITH COMPLEMENT */
> > +FbinOp(_ncrk,  asm("ncrk  %%r0, %%r3, %%r2\n" F_EPI))
> > +FbinOp(_ncgrk, asm("ncgrk %%r0, %%r3, %%r2\n" F_EPI))
>
> Better written as
>
>    asm("ncrk %0, %3, %2" : "=&r"(res) : "r"(a), "r"(b) : "cc");

I agree with Richard, especially since it's kind of "dangerous" to chain 
multiple asm() statements (without "volatile") and hoping that the compiler 
keeps the values in the registers in between (without reordering the 
statements).

Anyway, since I'll be away for most the rest of the week and we already have 
soft-freeze next week, I'd like to get this fixed for my pull request that I 
plan later for today or tomorrow, so I now went ahead and modified the code 
to look like this:

#define FbinOp(S, ASM) uint64_t S(uint64_t a, uint64_t b) \
{ \
    uint64_t res = 0; \
    asm ("llihf %[res],801\n" ASM \
         : [res]"=&r"(res) : [a]"r"(a), [b]"r"(b) : "cc"); \
    return res; \
}

/* AND WITH COMPLEMENT */
FbinOp(_ncrk,  ".insn rrf, 0xB9F50000, %[res], %[b], %[a], 0\n")
FbinOp(_ncgrk, ".insn rrf, 0xB9E50000, %[res], %[b], %[a], 0\n")

/* NAND */
FbinOp(_nnrk,  ".insn rrf, 0xB9740000, %[res], %[b], %[a], 0\n")
FbinOp(_nngrk, ".insn rrf, 0xB9640000, %[res], %[b], %[a], 0\n")

/* NOT XOR */
FbinOp(_nxrk,  ".insn rrf, 0xB9770000, %[res], %[b], %[a], 0\n")
FbinOp(_nxgrk, ".insn rrf, 0xB9670000, %[res], %[b], %[a], 0\n")

/* NOR */
FbinOp(_nork,  ".insn rrf, 0xB9760000, %[res], %[b], %[a], 0\n")
FbinOp(_nogrk, ".insn rrf, 0xB9660000, %[res], %[b], %[a], 0\n")

/* OR WITH COMPLEMENT */
FbinOp(_ocrk,  ".insn rrf, 0xB9750000, %[res], %[b], %[a], 0\n")
FbinOp(_ocgrk, ".insn rrf, 0xB9650000, %[res], %[b], %[a], 0\n")

Full patch can be seen here:

https://gitlab.com/thuth/qemu/-/commit/38af118ea2fef0c473

I hope that's ok for everybody?

 Thomas