Re: [Xen-devel] [PATCH] x86: use gcc6'es flags asm() output support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.16 at 18:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/07/16 17:10, Jan Beulich wrote:
>>>>> On 01.07.16 at 17:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> As for interleaving inside the asm statement itself, we already have
>>> precedent for that with the HAVE_GAS_* predicates.  It would make the
>>> patch rather larger, but might end up looking cleaner.  It is probably
>>> also worth switching to named parameters to reduce the risk of getting
>>> positional parameters out of order.
>> So taking just the first example I've converted: Do you think this
>>
>> static bool_t even_parity(uint8_t v)
>> {
>>     asm ( "test %1,%1"
>> #ifdef __GCC_ASM_FLAG_OUTPUTS__
>>           : "=@ccp" (v)
>> #else
>>           "; setp %0"
>>           : "=qm" (v)
>> #endif
>>           : "q" (v) );
>>
>>     return v;
>> }
>>
>> is better than the original?
> 
> How about a different example, from the second hunk
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 460d1f7..8d52a41 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -832,8 +832,19 @@ static int read_ulong(
>  static bool_t mul_dbl(unsigned long m[2])
>  {
>      bool_t rc;
> -    asm ( "mul %1; seto %2"
> -          : "+a" (m[0]), "+d" (m[1]), "=qm" (rc) );
> +
> +    asm ( "mul %1;"
> +#ifndef __GCC_ASM_FLAG_OUTPUTS__
> +          "seto %[rc];"
> +#endif
> +          : "+a" (m[0]), "+d" (m[1]),
> +#ifdef __GCC_ASM_FLAG_OUTPUTS__
> +            [rc] "=@cco" (rc)
> +#else
> +            [rc] "=qm" (rc)
> +#endif
> +        );
> +
>      return rc;
>  }
>  
> This at least doesn't mix the : inside an #ifdef

At the price of two #ifdef-s. And in the example I'm really not
worried about the colon going into both branches of the #if, but
about general readability of the resulting code.

>> I'm unsure, and I'm actually inclined to
>> think that then the abstraction alternative might look better.
> 
> If the abstraction comes in two parts, one which may insert a `setcc`
> instruction, and one which selects between =qm and =@cc, it wouldn't end
> up hiding the :.

Opening an easy route to making mistakes. Imo such an abstraction
needs to be either a single item, ot not be done at all.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel