| Message ID | 4D99FBCA.4030604@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/04/2011 01:11 PM, Aldy Hernandez wrote: > 1. Synchronized atomic load/stores: > > atomic_int atomi; > long double j; > > Thread 1: > j = 13.0; > atomi.store(1); > > Thread 2: > atomi.load(); > > As I understand it, the load/stores have acquire/release semantics, > so the store to <j> must happen before the store to <atomi>. Yep, that does seem to be true for the default memory_order_seq_cst. > Currently, this is not the case on x86-64, because GCC reorders the > stores and the attached atomics-2.C test fails. We emit the > following for thread 1: > > flds .LC0(%rip) > movl $1, atomi(%rip) > xorl %eax, %eax > fstpt j(%rip) > mfence > > Notice the store of <j> *after* the store to <atomi>. I consider > this a bug and have put this on my laundry list. Seems plausible, though I don't know the details of the x86_64 memory model well enough to be sure that it would be possible to observe this reordering. But then, if your test fails I guess it is. :) > 2. Unsynchronized atomic load/stores. > > For the "memory_order_relaxed" directive, load/stores are not > synchronized as the previous example, but stores to identical memory > locations must be performed in modification order. > > For the following loop, I assume and test that the stores to x[] do > not happen out of order: > > for (int i=0; i < SIZE; ++i) > x[i].store(666, memory_order_relaxed); > > This is the test in the attached atomics-3.C. I don't think that's testing what you want to test; those stores are to the same array, but not to the same memory location. Jason
>> Notice the store of <j> *after* the store to <atomi>. I consider >> this a bug and have put this on my laundry list. > > Seems plausible, though I don't know the details of the x86_64 memory > model well enough to be sure that it would be possible to observe this > reordering. But then, if your test fails I guess it is. :) I have not looked into why it happens, just that it does so incorrectly. So I'm just as stumped :). >> for (int i=0; i < SIZE; ++i) >> x[i].store(666, memory_order_relaxed); >> >> This is the test in the attached atomics-3.C. > > I don't think that's testing what you want to test; those stores are to > the same array, but not to the same memory location. Ahh, poop, yeah. I had a simpler test, but it's pretty hard to trick GCC to not eliminate the first store if they're both to the same location. I've removed this test, and only committed the first one. Thanks for looking into this. Aldy
Index: g++.dg/memmodel/atomics-2.C =================================================================== --- g++.dg/memmodel/atomics-2.C (revision 0) +++ g++.dg/memmodel/atomics-2.C (revision 0) @@ -0,0 +1,51 @@ +/* { dg-do link } */ +/* { dg-options "-std=c++0x -O2" } */ +/* { dg-final { memmodel-gdb-test } } */ + +using namespace std; + +#include <atomic> +#include <limits.h> +#include <stdio.h> +#include "memmodel.h" + +atomic_int atomi; + +/* Non-atomic. Use a type wide enough to possibly coerce GCC into + moving things around. */ +long double j; + + +/* Test that an atomic store synchronizes with an atomic load. + + In this case, test that the store to <j> happens-before the atomic + store to <atomi>. Make sure the compiler does not reorder the + stores. */ +main() +{ + j = 13.0; + atomi.store(1); + memmodel_done(); +} + +void memmodel_other_threads() +{ +} + +/* Verify that side-effects before an atomic store are correctly + synchronized with the an atomic load to the same location. */ +int memmodel_step_verify() +{ + if (atomi.load() == 1 && j != 13.0) + { + printf ("FAIL: invalid synchronization for atomic load/store.\n"); + return 1; + } + return 0; +} + + +int memmodel_final_verify() +{ + return memmodel_step_verify(); +} Index: g++.dg/memmodel/atomics-3.C =================================================================== --- g++.dg/memmodel/atomics-3.C (revision 0) +++ g++.dg/memmodel/atomics-3.C (revision 0) @@ -0,0 +1,55 @@ +/* { dg-do link } */ +/* { dg-options "-std=c++0x -O2" } */ +/* { dg-final { memmodel-gdb-test } } */ + +using namespace std; + +#include <atomic> +#include <limits.h> +#include <stdio.h> +#include "memmodel.h" + +#define SIZE 64 + +atomic_int x[SIZE]; // initially all 0's. +int i; + + +/* Test that atomic stores to the same location are performed in + modification order, despite the use of "memory_order_relaxed". */ +main() +{ + /* These stores must be done consecutively. */ + for (int i=0; i < SIZE; ++i) + x[i].store(666, memory_order_relaxed); + memmodel_done(); +} + +void memmodel_other_threads() +{ +} + +/* Verify that the stores were done in order, that is-- once you get + to an uninitialized value, every subsequent slot is empty as + well. */ +int memmodel_step_verify() +{ + bool seen0 = false; + for (i = 0; i < SIZE; ++i) + { + if (x[i].load() == 0) + seen0 = true; + else if (seen0) + { + printf ("FAIL: out-of-order modification\n"); + return 1; + } + } + return 0; +} + + +int memmodel_final_verify() +{ + return memmodel_step_verify(); +}
[Jason, Benjamin: It'd be nice to get input from y'all, since you are more intimately involved with the standard. I want to make sure I'm not misunderstanding things.] Hopefully a non-controversial feature, but given my luck, I doubt it :). Here are two additional tests to verify our atomic compliance. 1. Synchronized atomic load/stores: atomic_int atomi; long double j; Thread 1: j = 13.0; atomi.store(1); Thread 2: atomi.load(); As I understand it, the load/stores have acquire/release semantics, so the store to <j> must happen before the store to <atomi>. Currently, this is not the case on x86-64, because GCC reorders the stores and the attached atomics-2.C test fails. We emit the following for thread 1: flds .LC0(%rip) movl $1, atomi(%rip) xorl %eax, %eax fstpt j(%rip) mfence Notice the store of <j> *after* the store to <atomi>. I consider this a bug and have put this on my laundry list. 2. Unsynchronized atomic load/stores. For the "memory_order_relaxed" directive, load/stores are not synchronized as the previous example, but stores to identical memory locations must be performed in modification order. For the following loop, I assume and test that the stores to x[] do not happen out of order: for (int i=0; i < SIZE; ++i) x[i].store(666, memory_order_relaxed); This is the test in the attached atomics-3.C. The test atomics-2.C fails on x86-64, but atomics-3.C passes. I would like input on my interpretation of the proposed memory model standard, and the sanity of my tests. I will be committing the attached tests if there are no issues today (with the tests, not the sanity of the model :)). Aldy