| Message ID | 20160824204424.14041-2-bobby.prani@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 24/08/2016 22:44, Pranith Kumar wrote: > Use the __atomic_*_n() primitives which take the value as argument. It > is not necessary to store the value locally before calling the > primitive, hence saving us a stack store and load. If you do this, you might as well do it for __atomic_load and __atomic_compare_exchange. However, you'd still need typeof_strip_qual for __atomic_compare_exchange's "_old" local variable. The question is, does this actually cause a change in generated code? I would be very surprised if it did, but then it's perfectly possible to have a bug in GCC. Paolo
Paolo Bonzini writes: > On 24/08/2016 22:44, Pranith Kumar wrote: >> Use the __atomic_*_n() primitives which take the value as argument. It >> is not necessary to store the value locally before calling the >> primitive, hence saving us a stack store and load. > > If you do this, you might as well do it for __atomic_load and > __atomic_compare_exchange. However, you'd still need typeof_strip_qual > for __atomic_compare_exchange's "_old" local variable. OK, I will update the patch doing the same for load and compare exchange. > > The question is, does this actually cause a change in generated code? I > would be very surprised if it did, but then it's perfectly possible to > have a bug in GCC. It looks like it does. I tested atomic_load() with previous and the atomic_load_n() version and it generates the following code: current: mov 0x200c4a(%rip),%eax # 601030 <value> mov %eax,-0x4(%rsp) mov -0x4(%rsp),%eax atomic_load_n(): mov 0x200c4a(%rip),%eax # 601030 <value> So looks like it's worth it. Thanks,
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index d313e6b..fc2309f 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -103,8 +103,7 @@ #define atomic_set(ptr, i) do { \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val = (i); \ - __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \ + __atomic_store_n(ptr, i, __ATOMIC_RELAXED); \ } while(0) /* See above: most compilers currently treat consume and acquire the @@ -129,8 +128,7 @@ #define atomic_rcu_set(ptr, i) do { \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val = (i); \ - __atomic_store(ptr, &_val, __ATOMIC_RELEASE); \ + __atomic_store_n(ptr, i, __ATOMIC_RELEASE); \ } while(0) /* atomic_mb_read/set semantics map Java volatile variables. They are @@ -153,9 +151,8 @@ #define atomic_mb_set(ptr, i) do { \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val = (i); \ smp_wmb(); \ - __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \ + __atomic_store_n(ptr, i, __ATOMIC_RELAXED); \ smp_mb(); \ } while(0) #else @@ -169,8 +166,7 @@ #define atomic_mb_set(ptr, i) do { \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val = (i); \ - __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST); \ + __atomic_store_n(ptr, i, __ATOMIC_SEQ_CST); \ } while(0) #endif @@ -179,9 +175,7 @@ #define atomic_xchg(ptr, i) ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof_strip_qual(*ptr) _new = (i), _old; \ - __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \ - _old; \ + __atomic_exchange_n(ptr, i, __ATOMIC_SEQ_CST); \ }) /* Returns the eventual value, failed or not */
Use the __atomic_*_n() primitives which take the value as argument. It is not necessary to store the value locally before calling the primitive, hence saving us a stack store and load. Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- include/qemu/atomic.h | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)