diff mbox

[i386] Fix PR55981, atomic store is split in two smaller stores

Message ID CAFULd4aLW-NJ1QKoNfiJ3T+mnbEeMnV=xQPa7Pxa52CDZhsMug@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Jan. 16, 2013, 5:26 p.m. UTC 
Hello!

Using plain movdi pattern is not guaranteed to be atomic for all
operands. For x86_64, when storing DImode immediate outside SImode
range, the compiler splits the move into two separate SImode moves,
violating atomic assumptions.

Attached patch generates atomic store for all supported input arguments.

A related questions about volatile stores:
- Does language standard guarantee atomic store in this case
[wikipedia says "No." [1]]?
- Can a store to a volatile DImode location be implemented as two
consecutive SImode stores to adjacent location (this breaks stores to
MMIO 64bit registers)?

2012-01-16  Uros Bizjak  <ubizjak@gmail.com>

	PR target/55981
	* config/i386/sync.md (atomic_store<mode>): Always generate SWImode
	store through atomic_store<mode>_1.
	(atomic_store<mode>_1): Macroize insn using SWI mode iterator.

testsuite/ChangeLog:

2012-01-16  Uros Bizjak  <ubizjak@gmail.com>

	PR target/55981
	* gcc.target/pr55981.c: New test.

Tested on x86_64-pc-linux-gnu.

I will wait a couple of days for possible comments.

[1] http://en.wikipedia.org/wiki/Volatile_variable#In_C_and_C.2B.2B

Uros.

Comments

Richard Henderson Jan. 16, 2013, 5:39 p.m. UTC | #1 
On 01/16/2013 09:26 AM, Uros Bizjak wrote:
> 2012-01-16  Uros Bizjak<ubizjak@gmail.com>
>
> 	PR target/55981
> 	* config/i386/sync.md (atomic_store<mode>): Always generate SWImode
> 	store through atomic_store<mode>_1.
> 	(atomic_store<mode>_1): Macroize insn using SWI mode iterator.
>
> testsuite/ChangeLog:
>
> 2012-01-16  Uros Bizjak<ubizjak@gmail.com>
>
> 	PR target/55981
> 	* gcc.target/pr55981.c: New test.

This looks good to me.


r~
diff mbox

Patch

Index: config/i386/sync.md
===================================================================
--- config/i386/sync.md	(revision 195240)
+++ config/i386/sync.md	(working copy)
@@ -225,11 +225,8 @@ 
 	}
 
       /* Otherwise use a store.  */
-      if (INTVAL (operands[2]) & IX86_HLE_RELEASE)
-	emit_insn (gen_atomic_store<mode>_1 (operands[0], operands[1],
-					     operands[2]));
-      else
-	emit_move_insn (operands[0], operands[1]);
+      emit_insn (gen_atomic_store<mode>_1 (operands[0], operands[1],
+					   operands[2]));
     }
   /* ... followed by an MFENCE, if required.  */
   if (model == MEMMODEL_SEQ_CST)
@@ -238,10 +235,10 @@ 
 })
 
 (define_insn "atomic_store<mode>_1"
-  [(set (match_operand:ATOMIC 0 "memory_operand" "=m")
-	(unspec:ATOMIC [(match_operand:ATOMIC 1 "<nonmemory_operand>" "<r><i>")
-			(match_operand:SI 2 "const_int_operand")]
-		       UNSPEC_MOVA))]
+  [(set (match_operand:SWI 0 "memory_operand" "=m")
+	(unspec:SWI [(match_operand:SWI 1 "<nonmemory_operand>" "<r><i>")
+		     (match_operand:SWI 2 "const_int_operand")]
+		    UNSPEC_MOVA))]
   ""
   "%K2mov{<imodesuffix>}\t{%1, %0|%0, %1}")
 
Index: testsuite/gcc.target/i386/pr55981.c
===================================================================
--- testsuite/gcc.target/i386/pr55981.c	(revision 0)
+++ testsuite/gcc.target/i386/pr55981.c	(working copy)
@@ -0,0 +1,54 @@ 
+/* { dg-do compile  { target { ! { ia32 } } } } */
+/* { dg-options "-O2" } */
+
+volatile int a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p;
+
+volatile long long y;
+
+void
+test ()
+{
+  int a_ = a;
+  int b_ = b;
+  int c_ = c;
+  int d_ = d;
+  int e_ = e;
+  int f_ = f;
+  int g_ = g;
+  int h_ = h;
+  int i_ = i;
+  int j_ = j;
+  int k_ = k;
+  int l_ = l;
+  int m_ = m;
+  int n_ = n;
+  int o_ = o;
+  int p_ = p;
+
+  int z;
+
+  for (z = 0; z < 1000; z++)
+    {
+      __atomic_store_n (&y, 0x100000002ll, __ATOMIC_SEQ_CST);
+      __atomic_store_n (&y, 0x300000004ll, __ATOMIC_SEQ_CST);
+    }
+
+  a = a_;
+  b = b_;
+  c = c_;
+  d = d_;
+  e = e_;
+  f = f_;
+  g = g_;
+  h = h_;
+  i = i_;
+  j = j_;
+  k = k_;
+  l = l_;
+  m = m_;
+  n = n_;
+  o = o_;
+  p = p_;
+}
+
+/* { dg-final { scan-assembler-times "movabs" 2 } } */