After much pondering and talking with both you and Torvald, it has been
determined that the test at hand is technically allowed to hoist the
value of x/y because the standard guarantees that the code below is data
race free:
if (__atomic_load_n (&flag1, __ATOMIC_ACQUIRE))
i = x + y;
if (__atomic_load_n (&flag2, __ATOMIC_ACQUIRE))
a = 10;
j = x + y;
So, since j=x+y is an unconditional load of x/y, we can assume there are
no other threads writing to x/y.
However...
Depending on such undefined behaviors is liable to cause confusion and
frustration with our ourselves and our users, so it is best to limit
these optimizations across atomics altogether. It's best to err on the
side of caution than overly optimize across confusing data races. As we
become better versed in optimizing across atomics, we can relax things
and perhaps make optimizations more aggressive. But for now, let's mark
this as a must fix, and make sure hoists are not allowed across acquire
operations.
I have detailed the problem in the testcase.
> so enter it as 2 testcases, one increasing x and one increasing y, or
> better yet set it up so that this function is called twice from
> simulate_main, with other_process() increasing x the first time and
> increasing y the second time... or something like that.
>
> Andrew
Two testcases? Now you just want to see me work more :).
Implemented as loop.
OK for branch?
Index: atomic-hoist-1.c
===================================================================
--- atomic-hoist-1.c (revision 0)
+++ atomic-hoist-1.c (revision 0)
@@ -0,0 +1,89 @@
+/* { dg-do link } */
+/* { dg-require-effective-target sync_int_long } */
+/* { dg-final { simulate-thread } } */
+
+/* Test that a hoist is not performed across an acquire barrier. */
+
+#include <stdio.h>
+#include "simulate-thread.h"
+
+int iteration = 0;
+int flag1=1, flag2=1;
+unsigned int x=1, y=2, i=0x1234, j=0x5678, a;
+
+
+/* At each instruction, get a new X or Y to later verify that we have
+ not reused a value incorrectly. */
+void simulate_thread_other_threads ()
+{
+ if (iteration == 0)
+ x++;
+ else
+ y++;
+}
+
+/* Return true if error, otherwise 0. */
+int verify_result ()
+{
+ /* [i] should not equal [j], because that would mean that we hoisted
+ [x] or [y] instead of loading them again. */
+ int fail = i == j;
+ if (fail)
+ printf("FAIL: i (%u) should not equal j (%u)\n", i, j);
+ return fail;
+}
+
+int simulate_thread_step_verify ()
+{
+ return verify_result ();
+}
+
+int simulate_thread_final_verify ()
+{
+ return verify_result ();
+}
+
+__attribute__((noinline))
+void simulate_thread_main()
+{
+ for (; iteration < 2; ++iteration)
+ {
+ /* The values of x or y should not be hoisted across reads of
+ flag[12].
+
+ For example, when the second load below synchronizes with
+ another thread, the synchronization is with a release, and
+ that release may cause a stored value of x/y to be flushed
+ and become visible. So, for this case, it is incorrect for
+ CSE/CSA/and-others to hoist x or y above the load of
+ flag2. */
+ if (__atomic_load_n (&flag1, __ATOMIC_ACQUIRE))
+ i = x + y;
+ if (__atomic_load_n (&flag2, __ATOMIC_ACQUIRE))
+ a = 10;
+ /* NOTE: According to the standard we can assume that the
+ testcase is data race free, so if there is an unconditional
+ load of x+y here at j=x+y, there should not be any other
+ thread writing to x or y if we are indeed data race free.
+
+ This means that we are technically free to hoist x/y.
+ However, since depending on these undefined behaviors is
+ liable to get many confused, it is best to be conservative
+ with optimizations on atomics, hence the current test. As
+ we become better versed in optimizations across atomics, we
+ can relax the optimizations a bit. */
+ j = x + y;
+
+ /* Since x or y have been changing at each instruction above, i
+ and j should be different. If they are the same, we have
+ hoisted something incorrectly. */
+ }
+
+}
+
+main()
+{
+ simulate_thread_main ();
+ simulate_thread_done ();
+ return 0;
+}
