Re: [Xen-devel] [PATCH] x86/asm: Use ASM_FLAG_OUT() to simplify atomic and bitop stubs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 17:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/02/17 14:55, Jan Beulich wrote:
>>>>> On 15.02.17 at 15:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> bitops.h cannot include asm_defns.h, because the static inlines in 
> cpumasks.h
>>> result in forward declarations of the bitops.h contents.  Move 
> ASM_FLAG_OUT()
>>> to a new asm/compiler.h to compensate.
>>>
>>> While making changes, switch bool_t to bool and use named asm parameters.
>>>
>>> No (intended) functional change.  Without GCC flags, the disassembly is
>>> identical before and after this patch.  With GCC flags however, this patch
>>> seems to cause GCC to make instruction scheduling decisions.
>>>
>>> The first place with significant changes in the disassembly is the the 
> middle
>>> of do_domctl(), which switches from:
>>>
>>>     lock decl 0x1d8(%rax)
>>>     sete   %r14b
>>>     mov    $0xffffffffffffffea,%rbx
>>>     test   %r14b,%r14b
>>>     je     ffff82d0801034d4
>>>
>>> to:
>>>
>>>     lock decl 0x1d8(%rax)
>>>     mov    $0x0,%r14d
>>>     mov    $0xffffffffffffffea,%rbx
>>>     jne    ffff82d0801034d4
>>>
>>> avoiding the use of %r14d as an intermediate for calculating the conditional
>>> jump, freeing it up for later use.  As a result, the compiled code is a 
> little
>>> more efficient and smaller overall.
>> I have to admit that I find this rather surprising, especially since
>> (leaving the naming of the asm() operands aside) the preprocessed
>> source should not change afaict. It being unexpected I think calls
>> for better understanding what's going on here.
> 
> I presume you are happy now given your other discovery?

Yes, albeit the commit message may want refining a little.

>>> --- a/xen/include/asm-x86/config.h
>>> +++ b/xen/include/asm-x86/config.h
>>> @@ -107,6 +107,7 @@ extern unsigned char boot_edid_info[128];
>>>  #define asmlinkage
>>>  
>>>  #include <xen/const.h>
>>> +#include <asm/compiler.h>
>> Why here? You need it in bitops.h and maybe (it's not really used
>> there) asm_defns.h only for now.
> 
> I am looking to avoid duplicating the definition, and this seemed like
> an appropriate place to live.
> 
> Would you instead prefer me to duplicate it in bitops?

Duplicate what? The macro? No. The new header is fine, just that
I'd rather not see it included from config.h. And btw., other than
when Sergey wanted to put in a non-x86 header, instead of
introducing asm/compiler.h here, I think I would then agree to
simply put it in xen/compiler.h and be done once and for all.

Jan


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