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

* Re: CRIS atomics revisited 4/4: give up on alignment of atomic data
  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 ` 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
  1 sibling, 0 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-07-16  6:12 UTC (permalink / raw)
  To: gcc-patches

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Mon, 16 Jul 2012 05:49:00 +0200

> gcc:

> 	* 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.

A sync goof (the VC kind): the committed and sent patch, but
not the changelog, was missing the first hunk, now committed:

Index: config/cris/sync.md
===================================================================
--- config/cris/sync.md	(revision 189504)
+++ config/cris/sync.md	(working copy)
@@ -101,7 +101,7 @@ (define_expand "atomic_fetch_<atomic_op_
    (match_operand:BWD 2 "<atomic_op_op_pred>")
    (match_operand 3)
    (atomic_op:BWD (match_dup 0) (match_dup 1))]
-  ""
+  "<MODE>mode == QImode || !TARGET_ATOMICS_MAY_CALL_LIBFUNCS"
 {
   enum memmodel mmodel = (enum memmodel) INTVAL (operands[3]);
 

brgds, H-P

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

* Re: CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook
  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 ` Andrew MacLeod
  2012-07-17 16:26   ` Hans-Peter Nilsson
  2012-07-17 22:55   ` Hans-Peter Nilsson
  1 sibling, 2 replies; 6+ messages in thread
From: Andrew MacLeod @ 2012-07-17 12:25 UTC (permalink / raw)
  To: Hans-Peter Nilsson; +Cc: gcc-patches

On 07/15/2012 11:49 PM, Hans-Peter Nilsson wrote:
> 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.

I set it up to eventually handle alignment issues, but didn't really 
know any details of what to do when, or how.  Input was rather lacking 
at the time and there was a time crunch, so I just punted on it in 4.7 
at the tree level and targets did their own thing.  The library idea was 
new enough that the standards guys couldnt/wouldn't give me a straight 
answer since it hadn't really been thought through I don't think.  My 
apologies if I misrepresent anyone there :-)

The basic problem you are dealing with is a 4 byte object that is not 
aligned properly, so the  lock-free instructions don't work on it., but 
'always_lock_free' says "yes, that type is lock free". Then in the 
library, its implemented via a lock?  Is that approximately it?  or is 
there an additional issue?

The original intention was that  __atomic{is,always}_lock_free takes the 
size and a pointer to the object, and the alignment of the pointer 
object would be checked at compile time to see if it is always 
compatible with size. (so a char pointer to a 4 byte object would fail 
that test). which would means 'always_lock_free' should return false, 
and in turn a call to 'is_lock_free' would generate a library call and 
check the alignment at runtime of the object to determine what to do.    
The library implementation of all the other __atomic operations would 
check 'is_lock_free' to see whether they need to use a lock, or whether 
they can use available lock-free sequences/syscalls.

Some confusion around the standard surfaced and the intention from the 
standard point of view appeared to be that 'is_lock_free' should be true 
or false ALL the time for a given type.    This possibly is related to 
the separation of gcc's built-ins from the implementation details of the 
standard  (ie the standard uses the built-ins but only with aligned 
objects, but those built-ins can also still be used outside the standard 
in more chaotic ways)

So in the end, i wasn't sure what to do and ran out of time.  Making it 
better for 4.8 would be goodness.  Is there a fatal flaw in the original 
(unimplemented) intention?  if so, can it be fixed without changing the API?

Any PR's you open related this this, copy me on them and I'll try to get 
them addressed.

Andrew


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

* Re: CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook
  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
  1 sibling, 0 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-07-17 16:26 UTC (permalink / raw)
  To: amacleod; +Cc: gcc-patches

> From: Andrew MacLeod <amacleod@redhat.com>
> Date: Tue, 17 Jul 2012 14:24:48 +0200

> On 07/15/2012 11:49 PM, Hans-Peter Nilsson wrote:
> > 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.
> 
> I set it up to eventually handle alignment issues, but didn't really 
> know any details of what to do when, or how.  Input was rather lacking 
> at the time and there was a time crunch, so I just punted on it in 4.7 
> at the tree level and targets did their own thing.

Most (all but mine :) seem happy to provide naturally-aligned
types and promising to never access non-naturally-aligned data
so not much they need to do...  Not sure if the i386 ABI (or
e.g. xchg and similar insns in the CPU) promises or requires
that all data (ints) are aligned or if something is nominally
required.

>  The library idea was 
> new enough that the standards guys couldnt/wouldn't give me a straight 
> answer since it hadn't really been thought through I don't think.  My 
> apologies if I misrepresent anyone there :-)
> 
> The basic problem you are dealing with is a 4 byte object that is not 
> aligned properly, so the  lock-free instructions don't work on it., but 
> 'always_lock_free' says "yes, that type is lock free". Then in the 
> library,

(The library here meaning the atomicity implementation, not
e.g. libstdc++.  I assume you don't mean libatomic, which seems
to be intended just as a fallback for targets and sizes
unsupported by core gcc, so they're guaranteed to not be
lock-free.)

> its implemented via a lock?  Is that approximately it?  or is 
> there an additional issue?

That's pretty much it...  except it'd be better letting the
target decorate the intended atomic type, e.g. with alignment
attributes.  See suggestion last.  Decoration is in place for
_Atomic_word but I can't tell how _Atomic_word relates to the
issues here.  Is _Atomic_word just obsolete?

> The original intention was that  __atomic{is,always}_lock_free takes the 
> size and a pointer to the object, and the alignment of the pointer 
> object would be checked at compile time to see if it is always 
> compatible with size.

The alignment of the pointed-to *object* can't be checked at
compile-time.  As GCC is pretty bad at alignment, taking on the
task of trying to make a difference for different *types* seems
non-trivial.  All you can hope for is telling apart the size of
the objects.

Alignment of objects *can* be checked at run-time from within
the current __atomic_is_lock_free API, and apparently that's the
intention in the core gcc implementation, but the last few
details aren't there or are wrong.

> (so a char pointer to a 4 byte object would fail 
> that test). which would means 'always_lock_free' should return false, 
> and in turn a call to 'is_lock_free' would generate a library call and 
> check the alignment at runtime of the object to determine what to do.    

For the record, is_lock_free in libstdc++ calls
__atomic_is_lock_free, which is per-instance at the API level.
I don't see usage of __atomic_always_lock_free in libstdc++.
(I see it in libatomic/glfree.c:libat_is_free, but dominated by
a check for natural alignment of the pointer instance with
failure yielding false, bravo.)

Not using __atomic_always_lock_free is probably wrong, it seems
it should call __atomic_always_lock_free if I understand
/path/to/trunk/libstdc++-v3/doc/html/ext/lwg-closed.html#1146
correctly (the link to www.open-std.org there is dead).  Can
this be confirmed?  Still wouldn't work of course - the target
can't doesn't have a say in the __atomic_always_lock_free
implementation.

> The library implementation of all the other __atomic operations would 
> check 'is_lock_free' to see whether they need to use a lock, or whether 
> they can use available lock-free sequences/syscalls.

(You must mean libstdc++ here which is slightly confusing with
the use of "__atomic_" which for me implies the gcc
implementation.)

> Some confusion around the standard surfaced and the intention from the 
> standard point of view appeared to be that 'is_lock_free' should be true 
> or false ALL the time for a given type.

And there's confusion all around telling apart atomic access
from *lock-free* atomic access. :-)  Witness
__GCC_ATOMIC_INT_LOCK_FREE and the md.texi note: "For the
purposes of C++11 @code{std::atomic::is_lock_free}, it is
assumed that these library calls do @emph{not} use any kind of
interruptable locking".

Come to think of it, what is "interruptable locking"?
(More STFW hits for "interruptible locking" FWIW.)
I don't remember in which of the search hits I read it, but I
came to the conclusion that if there exists a place where a
signal can hit a thread such that *other* threads would be
halted, then operations are not lock-free.  (For example if you
single-step for a thread accessing an object in gdb, at no point
should it be possible for other threads to hang "spinning" while
trying to do atomic access on the object.)

I can imagine a more weasly-worded definition, but then I
wouldn't see the difference between an instruction locking a
word or a cache-line (excluding other threads and possibly
memory-accessing machinery) while performing the atomicity, to
that of a library function acquiring a common misalignment-lock
for the duration of the atomic operation.

>    This possibly is related to 
> the separation of gcc's built-ins from the implementation details of the 
> standard  (ie the standard uses the built-ins but only with aligned 

"the standard" meaning libstdc++ I presume.
Let's try and avoid confusing gcc core atomics with libstdc++
needs; let's say gcc and libstdc++.

> objects, but those built-ins can also still be used outside the standard 
> in more chaotic ways)

You mean libstdc++ applies is_atomic only to _Atomic_word
things? ...looking... no, IIUC.  Is this confusion important?

> So in the end, i wasn't sure what to do and ran out of time.  Making it 
> better for 4.8 would be goodness.  Is there a fatal flaw in the original 
> (unimplemented) intention?  if so, can it be fixed without changing the API?

Which API?

The gcc builtins API to "user" source code (i.e. to libstdc++)?
Sure.  Some values may change; __GCC_ATOMIC_INT_LOCK_FREE would
be 1 ("no...?"), not 2 ("Sure!").  (Weird that 0 isn't used, but
you probably already noticed that. :)

The libstdc++ API to its users?  Sure, but it seems the
libstdc++ code should be changed to use
__atomic_always_lock_free (see above).

The gcc target back-end API?  Maybe... but only of code is
changed to default to return false for __atomic_always_lock_free
and __atomic_is_lock_free, when the underlying type has less
than natural alignment (don't confuse this with comparing
against the *default* alignment, that confusion is already
implemented).  For __atomic_is_lock_free, a new hook, maybe even
a target MD pattern, is recommended to avoid having to err on
the safe side.  Or shorter, for __atomic_is_lock_free, change it
to do what's in libatomic/glfree.c:libat_is_free.

An improvement affecting all APIs would be a (new) __attribute__
((__atomic_lock_free__)), where the target gets to add things
like alignment to make lock-free atomicity certain to the
object, or make compilation fail.

> Any PR's you open related this this, copy me on them and I'll try to get 
> them addressed.

Thanks in advance.  I'm not too well-versed with trees, or I
might have a try at presumed one-liners.

brgds, H-P

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

* Re: CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook
  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
  1 sibling, 1 reply; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-07-17 22:55 UTC (permalink / raw)
  To: amacleod; +Cc: gcc-patches

> From: Andrew MacLeod <amacleod@redhat.com>
> Date: Tue, 17 Jul 2012 14:24:48 +0200

> Any PR's you open related this this, copy me on them and I'll try to get 
> them addressed.

I could separate the issues I saw into PRs 54003-6.  That's all,
hopefully ...at least for now. :)

BTW, your @gcc.gnu.org account doesn't have a bugzilla account
("did not match anything" says bugzilla), contrary to most/all
other accounts.  Maybe deliberate or known, maybe something to
look into.

brgds, H-P

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

* Re: CRIS atomics revisited 4/4: give up on alignment of atomic data, RFC for is_lock_free hook
  2012-07-17 22:55   ` Hans-Peter Nilsson
@ 2012-07-17 23:02     ` Andrew MacLeod
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew MacLeod @ 2012-07-17 23:02 UTC (permalink / raw)
  To: Hans-Peter Nilsson; +Cc: gcc-patches

On 07/17/2012 06:55 PM, Hans-Peter Nilsson wrote:
>> From: Andrew MacLeod <amacleod@redhat.com>
>> Date: Tue, 17 Jul 2012 14:24:48 +0200
>> Any PR's you open related this this, copy me on them and I'll try to get
>> them addressed.
> I could separate the issues I saw into PRs 54003-6.  That's all,
> hopefully ...at least for now. :)

Thanks, I'll have a look at them shortly.
> BTW, your @gcc.gnu.org account doesn't have a bugzilla account
> ("did not match anything" says bugzilla), contrary to most/all
> other accounts.  Maybe deliberate or known, maybe something to
> look into.
>
>
known, but never got around to investigating since it hasn't had a 
determinable effect  :-)

Andrew

^ 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).