public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook
@ 2012-07-16  3:49 Hans-Peter Nilsson
  2012-07-16  6:12 ` CRIS atomics revisited 4/4: give up on alignment of atomic data Hans-Peter Nilsson
  2012-07-17 12:25 ` CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook Andrew MacLeod
  0 siblings, 2 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-07-16  3:49 UTC (permalink / raw)
  To: gcc-patches

Well, give up by default that is, and fix it up in a helper
function in glibc to hold a global byte-sized atomic lock for
the duration.  (Sorry!)  Yes, this means that
fold_builtin_atomic_always_lock_free is wrong.  It knows about
alignment in general but doesn't handle the case where the
default alignment of the underlying type is too small for atomic
accesses, and should probably be augmented by a target hook,
alternatively, change the allow_libcall argument in the call to
can_compare_and_swap_p to false.  I guess I should open a PR for
this and add a test-case.  Later.

Too many library API writers don't cater for the possibility
that atomic (lockless) data may need to have certain properties
that may not be matched by the basic underlying data type,
specifically alignment, and fixing the failing instances by hand
is...challenging.  About half the cases have the atomic data
defined in the proximity of the atomic operations, and are
easily locally fixable.  The other half are of increasing
complexity; may have the data defined elsewhere, where the need
for atomicity is surprising and fixing it would be a kludge.
(But with a proper API could be easily handled, e.g. if a
data-type defined specific for the purpose was used; one
different than the underlying type or other common derivated
type used in the library.)

So, we'll change things for cris*-linux.  By default, call a
helper function.  Users can change the default at the caller
site where atomic alignment is known good or where there is
interest in fixing it up when failure is seen; executing a trap
insn was the old default.

Regarding changing fold_builtin_atomic_always_lock_free or
adding a hook, I posit that a better default is to assume atomic
data has to be naturally aligned on *all* existing GCC targets
to accomplish lockless operations, at least for those that don't
just punt to a system call, so maybe
fold_builtin_atomic_always_lock_free should be changed to check
that, by default.  Now, it just assumes that the default
type-alignment is ok and that only a smaller alignment is not
always atomic.  People with counter-examples are asked to please
explain how the counter-example handles data straddling a page
boundary.  Yes, it can be done, but how does the kernel-equivalent
accomplish atomicity; are the pages locked while the instruction
(assumed to cause an exception), is emulated, or is kernel
re-entrance impossible or what?

The default remains the same for non-*-linux-* cris-* and
crisv32-* subtargets, since the code compiled for those targets
is expected to have a different focus, one where fixing
non-aligned data definitions is feasible and desirable.

I deliberately make it optional and use weasel-wording whether
the library functions are actually called or an atomic insn
sequence emitted when suitable; when GCC knows the alignment of
the data (for example, for local static data or through
deliberate attributes) it should be allowed to emit the atomic
instruction sequence even without alignment checks.  Right now
(or maybe it was just the 4.7 branch), GCC's handling of
alignment is so poor that the emitted alignment checks (those
that conditionally execute a trap insn) aren't eliminated for
atomic data with explicit large-enough attribute declarations.
From what I (with limited tree-foo) could see, IIRC basically
everything about alignment for the specific data is discarded
and the underlying default type alignment is reported.  (Right,
a PR is in order; I know I've entered a PR for the related
__alignof__.)

gcc:
	* config/cris/cris.c (cris_init_libfuncs): Handle initialization
	of library functions for basic atomic compare-and-swap.
	* config/cris/cris.h (TARGET_ATOMICS_MAY_CALL_LIBFUNCS): New macro.
	* config/cris/cris.opt (munaligned-atomic-may-use-library): New
	option.
	* config/cris/sync.md ("atomic_fetch_<atomic_op_name><mode>")
	("cris_atomic_fetch_<atomic_op_name><mode>_1")
	("atomic_compare_and_swap<mode>")
	("cris_atomic_compare_and_swap<mode>_1"): Make
	conditional on TARGET_ATOMICS_MAY_CALL_LIBFUNCS for
	sizes larger than byte.

gcc/testsuite:
	* gcc.target/cris/sync-2i.c, gcc.target/cris/sync-2s.c,
	gcc.target/cris/sync-3i.c, gcc.target/cris/sync-3s.c,
	gcc.target/cris/sync-4i.c, gcc.target/cris/sync-4s.c,
	gcc.target/cris/sync-1-v10.c,
	gcc.target/cris/sync-1-v32.c: For cris*-*-linux*, also
	pass -mno-unaligned-atomic-may-use-library.
	* gcc.target/cris/sync-xchg-1.c: New test.

diff --git gcc/config/cris/cris.c gcc/config/cris/cris.c
index 22b254f..e4c11fd 100644
--- gcc/config/cris/cris.c
+++ gcc/config/cris/cris.c
@@ -3130,6 +3176,16 @@ cris_init_libfuncs (void)
   set_optab_libfunc (udiv_optab, SImode, "__Udiv");
   set_optab_libfunc (smod_optab, SImode, "__Mod");
   set_optab_libfunc (umod_optab, SImode, "__Umod");
+
+  /* Atomic data being unaligned is unfortunately a reality.
+     Deal with it.  */
+  if (TARGET_ATOMICS_MAY_CALL_LIBFUNCS)
+    {
+      set_optab_libfunc (sync_compare_and_swap_optab, SImode,
+			 "__cris_atcmpxchgr32");
+      set_optab_libfunc (sync_compare_and_swap_optab, HImode,
+			 "__cris_atcmpxchgr16");
+    }
 }
 
 /* The INIT_EXPANDERS worker sets the per-function-data initializer and
diff --git gcc/config/cris/cris.h gcc/config/cris/cris.h
index dee9d07..6bb457d 100644
--- gcc/config/cris/cris.h
+++ gcc/config/cris/cris.h
@@ -324,6 +324,11 @@ extern int cris_cpu_version;
 #define TARGET_TRAP_USING_BREAK8 \
  (cris_trap_using_break8 == 2 ? TARGET_HAS_BREAK : cris_trap_using_break8)
 
+/* Call library functions by default for GNU/Linux.  */
+#define TARGET_ATOMICS_MAY_CALL_LIBFUNCS		\
+ (cris_atomics_calling_libfunc == 2			\
+  ? TARGET_LINUX : cris_atomics_calling_libfunc)
+
 /* The < v10 atomics turn off interrupts, so they don't need alignment.
    Incidentally, by default alignment is off there causing variables to
    be default unaligned all over, so we'd have to make support
diff --git gcc/config/cris/cris.opt gcc/config/cris/cris.opt
index 2d12a5c..cd58004 100644
--- gcc/config/cris/cris.opt
+++ gcc/config/cris/cris.opt
@@ -183,6 +183,10 @@ mtrap-unaligned-atomic
 Target Report Var(cris_trap_unaligned_atomic) Init(2)
 Emit checks causing \"break 8\" instructions to execute when applying atomic builtins on misaligned memory
 
+munaligned-atomic-may-use-library
+Target Report Var(cris_atomics_calling_libfunc) Init(2)
+Handle atomic builtins that may be applied to unaligned data by calling library functions. Overrides -mtrap-unaligned-atomic.
+
 ; TARGET_SVINTO: Currently this just affects alignment.  FIXME:
 ; Redundant with TARGET_ALIGN_BY_32, or put machine stuff here?
 ; This and the others below could just as well be variables and
diff --git gcc/config/cris/sync.md gcc/config/cris/sync.md
index 558afbf..c465ce5 100644
--- gcc/config/cris/sync.md
+++ gcc/config/cris/sync.md
@@ -110,3 +125,3 @@
    (clobber (match_scratch:SI 3 "=&r"))]
-  ""
+  "<MODE>mode == QImode || !TARGET_ATOMICS_MAY_CALL_LIBFUNCS"
 {
@@ -189,3 +205,3 @@
    (match_operand 7)]
-  ""
+  "<MODE>mode == QImode || !TARGET_ATOMICS_MAY_CALL_LIBFUNCS"
 {
@@ -217,3 +233,3 @@
 	 CRIS_UNSPEC_ATOMIC_SWAP_MEM))]
-  ""
+  "<MODE>mode == QImode || !TARGET_ATOMICS_MAY_CALL_LIBFUNCS"
 {
diff --git gcc/testsuite/gcc.target/cris/sync-2i.c gcc/testsuite/gcc.target/cris/sync-2i.c
index e43aa53..d491d3c 100644
--- gcc/testsuite/gcc.target/cris/sync-2i.c
+++ gcc/testsuite/gcc.target/cris/sync-2i.c
@@ -2,6 +2,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dop -Dtype=int" } */
 /* { dg-additional-options "-mtrap-using-break8 -mtrap-unaligned-atomic" { target cris-*-elf } } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler "\tbreak 8" } } */
 /* { dg-final { scan-assembler "\tbtstq \\(2-1\\)," } } */
 /* { dg-final { scan-assembler-not "\tand" } } */
diff --git gcc/testsuite/gcc.target/cris/sync-2s.c gcc/testsuite/gcc.target/cris/sync-2s.c
index 9be7dc6..06ff98a 100644
--- gcc/testsuite/gcc.target/cris/sync-2s.c
+++ gcc/testsuite/gcc.target/cris/sync-2s.c
@@ -2,6 +2,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dop -Dtype=short" } */
 /* { dg-additional-options "-mtrap-using-break8 -mtrap-unaligned-atomic" { target cris-*-elf } } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler "\tbreak 8" } } */
 /* { dg-final { scan-assembler "\tbtstq \\(1-1\\)," } } */
 /* { dg-final { scan-assembler-not "\tand" } } */
diff --git gcc/testsuite/gcc.target/cris/sync-3i.c gcc/testsuite/gcc.target/cris/sync-3i.c
index 114e0a8..9e67d61 100644
--- gcc/testsuite/gcc.target/cris/sync-3i.c
+++ gcc/testsuite/gcc.target/cris/sync-3i.c
@@ -4,6 +4,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dxchg -Dtype=int" } */
 /* { dg-additional-options "-mtrap-using-break8 -mtrap-unaligned-atomic" { target cris-*-elf } } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler "\tbreak 8" } } */
 /* { dg-final { scan-assembler "\tbtstq \\(2-1\\)," { xfail *-*-* } } } */
 /* { dg-final { scan-assembler-not "\tand" { xfail *-*-* } } } */
diff --git gcc/testsuite/gcc.target/cris/sync-3s.c gcc/testsuite/gcc.target/cris/sync-3s.c
index facbb39..8e87a3b 100644
--- gcc/testsuite/gcc.target/cris/sync-3s.c
+++ gcc/testsuite/gcc.target/cris/sync-3s.c
@@ -4,6 +4,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dxchg -Dtype=short" } */
 /* { dg-additional-options "-mtrap-using-break8 -mtrap-unaligned-atomic" { target cris-*-elf } } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler "\tbreak 8" } } */
 /* { dg-final { scan-assembler "\tbtstq \\(1-1\\)," { xfail *-*-* } } } */
 /* { dg-final { scan-assembler-not "\tand" { xfail *-*-* } } } */
diff --git gcc/testsuite/gcc.target/cris/sync-4i.c gcc/testsuite/gcc.target/cris/sync-4i.c
index 9756c69..78a7012 100644
--- gcc/testsuite/gcc.target/cris/sync-4i.c
+++ gcc/testsuite/gcc.target/cris/sync-4i.c
@@ -1,6 +1,7 @@
 /* Check that we don't get alignment-checking code, int.  */
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dtype=int -mno-trap-unaligned-atomic" } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler-not "\tbreak\[ \t\]" } } */
 /* { dg-final { scan-assembler-not "\tbtstq\[ \t\]\[^5\]" } } */
 /* { dg-final { scan-assembler-not "\tand" } } */
diff --git gcc/testsuite/gcc.target/cris/sync-4s.c gcc/testsuite/gcc.target/cris/sync-4s.c
index 2d64430..6691a48 100644
--- gcc/testsuite/gcc.target/cris/sync-4s.c
+++ gcc/testsuite/gcc.target/cris/sync-4s.c
@@ -1,6 +1,7 @@
 /* Check that we don't get alignment-checking code, short.  */
 /* { dg-do compile } */
 /* { dg-options "-O2 -Dtype=short -mno-trap-unaligned-atomic" } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 /* { dg-final { scan-assembler-not "\tbreak\[ \t\]" } } */
 /* { dg-final { scan-assembler-not "\tbtstq\[ \t\]\[^5\]" } } */
 /* { dg-final { scan-assembler-not "\tand" } } */
diff --git gcc/testsuite/gcc.target/cris/sync-1-v10.c gcc/testsuite/gcc.target/cris/sync-1-v10.c
index 640098a..6c8dd1a 100644
--- gcc/testsuite/gcc.target/cris/sync-1-v10.c
+++ gcc/testsuite/gcc.target/cris/sync-1-v10.c
@@ -1,4 +1,5 @@
 /* Check that we can assemble both base atomic variants.  */
 /* { dg-do assemble } */
 /* { dg-options "-O2 -march=v10" } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 #include "sync-1.c"
diff --git gcc/testsuite/gcc.target/cris/sync-1-v32.c gcc/testsuite/gcc.target/cris/sync-1-v32.c
index 644d922..3c1d076 100644
--- gcc/testsuite/gcc.target/cris/sync-1-v32.c
+++ gcc/testsuite/gcc.target/cris/sync-1-v32.c
@@ -1,4 +1,5 @@
 /* Check that we can assemble both base atomic variants.  */
 /* { dg-do assemble } */
 /* { dg-options "-O2 -march=v32" } */
+/* { dg-additional-options "-mno-unaligned-atomic-may-use-library" { target cris*-*-linux* } } */
 #include "sync-1.c"
Index: gcc.target/cris/sync-xchg-1.c
===================================================================
--- gcc.target/cris/sync-xchg-1.c	(revision 0)
+++ gcc.target/cris/sync-xchg-1.c	(revision 0)
@@ -0,0 +1,21 @@
+/* Check that the basic library call variant is sane; no other calls, no
+   checks compares or branches.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -munaligned-atomic-may-use-library" } */
+/* { dg-final { scan-assembler-not "\tdi" } } */
+/* { dg-final { scan-assembler-not "\tbtstq" } } */
+/* { dg-final { scan-assembler-not "\tand" } } */
+/* { dg-final { scan-assembler-not "\tclearf" } } */
+/* { dg-final { scan-assembler-not "\tmove.d" } } */
+/* { dg-final { scan-assembler-not "\tcmp" } } */
+/* { dg-final { scan-assembler-not "\tb\[^s\]" } } */
+/* { dg-final { scan-assembler-times "\t\[JjBb\]sr" 1 } } */
+
+#ifndef type
+#define type int
+#endif
+
+type svcsw (type *ptr, type oldval, type newval)
+{
+  return __sync_val_compare_and_swap (ptr, oldval, newval);
+}

brgds, H-P

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-07-17 23:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-16  3:49 CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook Hans-Peter Nilsson
2012-07-16  6:12 ` CRIS atomics revisited 4/4: give up on alignment of atomic data Hans-Peter Nilsson
2012-07-17 12:25 ` CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook Andrew MacLeod
2012-07-17 16:26   ` Hans-Peter Nilsson
2012-07-17 22:55   ` Hans-Peter Nilsson
2012-07-17 23:02     ` Andrew MacLeod

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).