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

[David Hildenbrand]

On 28.02.22 11:14, Thomas Huth wrote:
> 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?

Fine with me.


-- 
Thanks,

David / dhildenb