We have 2 choices:

1. Update document of

`TYPE __sync_fetch_and_add (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_fetch_and_sub (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_fetch_and_or (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_fetch_and_and (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_fetch_and_xor (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_fetch_and_nand (TYPE *ptr, TYPE value, ...)'

to

 {
   tmp = *ptr;
   tmp1 = tmp OP value;
   return __sync_val_compare_and_swap (ptr, tmp, tmp1);
 }

2. Remove those __sync_fetch_and_XXX which aren't

 { tmp = *ptr; *ptr OP= value; return tmp; }

Since only

 { tmp = *ptr; *ptr OP= value; return tmp; }

can be used to implement locks, I think we should do 2.

Technically it is closer to 

do { 
  tmp = *ptr;
  tmp1 = tmp OP value;
} while (__sync_val_compare_and_swap (ptr, tmp, tmp1) != tmp);
return tmp;

except it uses the new value of tmp from the CAS rather than reloading it again.

It loops performing OP on the new value of tmp until the CAS is successful. Functionally it is identical, what is your case where it can't be used?

The only difference I see is that another very hostile process which constantly changes *ptr may prevent forward progress from ever happening, resulting in an infinite loop.  

At the moment we aren't making any guarantees of forward progress, although I think at some point we may want to add a flag to the compiler as it becomes more of an issue.   With a flag like that enabled, a CAS loop would not be a valid implementation.

The same problem with

`TYPE __sync_add_and_fetch (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_sub_and_fetch (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_or_and_fetch (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_and_and_fetch (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_xor_and_fetch (TYPE *ptr, TYPE value, ...)'
`TYPE __sync_nand_and_fetch (TYPE *ptr, TYPE value, ...)'
     These builtins perform the operation suggested by the name, and
     return the new value.  That is,

          { *ptr OP= value; return *ptr; }
          { *ptr = ~(*ptr & value); return *ptr; }   // nand

[hjl@gnu-33 pr50583]$ cat z.i
int
foo (int *p)
{
  return __sync_and_and_fetch (p, 7);
}
[hjl@gnu-33 pr50583]$ gcc -O2 -S z.i
[hjl@gnu-33 pr50583]$ cat z.s
	.file	"z.i"
	.text
	.p2align 4,,15
	.globl	foo
	.type	foo, @function
foo:
.LFB0:
	.cfi_startproc
	movl	(%rdi), %eax
.L2:
	movl	%eax, %edx
	andl	$7, %edx
	lock cmpxchgl	%edx, (%rdi)
	jne	.L2
	movl	%edx, %eax
	ret
	.cfi_endproc
.LFE0:
	.size	foo, .-foo
	.ident	"GCC: (GNU) 4.6.0 20110603 (Red Hat 4.6.0-10)"
	.section	.note.GNU-stack,"",@progbits
[hjl@gnu-33 pr50583]$ 

It is

 {
   tmp = *ptr;
   do
     {
       tmp1 = tmp OP value;
     }
   while (__sync_val_compare_and_swap (ptr, tmp, tmp1) != tmp);
   return tmp1;
 }

If *ptr is changed between load and cmpxchg, we get an infinite loop.

I guess it is OK.

(In reply to comment #3)

>     movl    (%rdi), %eax
> .L2:
>     movl    %eax, %edx
>     andl    $7, %edx
>     lock cmpxchgl    %edx, (%rdi)
>     jne    .L2
>     movl    %edx, %eax

> It is
> 
>  {
>    tmp = *ptr;
>    do
>      {
>        tmp1 = tmp OP value;
>      }
>    while (__sync_val_compare_and_swap (ptr, tmp, tmp1) != tmp);
>    return tmp1;
>  }
> 
> If *ptr is changed between load and cmpxchg, we get an infinite loop.

No, you aren't translating the asm correctly back into C.
It is
  tmp = *ptr;
  do
    tmp1 = tmp OP value; tmp2 = tmp;
  while ((tmp = __sync_val_compare_and_swap (ptr, tmp2, tmp1) != tmp2);
  return tmp1;
because cmpxchgl instruction loads the *ptr value from memory into %eax if the instruction has been unsuccessful.  So, tmp is loaded with the new value for the next iteration.

Can't say I'm a fan of adding such a heavy weight sequence into
an intrinsic.  Maybe better to simply leave out the intrinsics that
cannot be implemented with loops? If someone wants a loop they
better open code it.

It would be nice if you implemented the ors, nands and ands
with bts, btr etc if the second argument is a constant and only has one
bit set.