Re: [PATCH] Fix errors in expand_atomic_store.
On 11/01/2011 04:48 PM, Richard Henderson wrote: On 11/01/2011 01:20 PM, David Miller wrote: Unfortunately, this is not true. Otherwise we could change the 32-bit default code generation to v9 from v7 under Linux. For v7, pa-risc, and sh, we originally allowed the test_and_set and lock_release patterns to do non-obvious things with 0/1 constants. My proposal is to *not* carry that over to the __atomic patterns. The PA and SH targets have already switched to use kernel helpers, and no longer rely on this hack. The only one left is Sparc v7. C++11 and C1x atomics do have an atomic_flag object which implements test _and_set and clear on a flag, and the standard requires that these *are* lock free . I was hoping to avoid introducing new routines just for that object and using the store and exchange since they are basically the same thing. If this is going to be a big issue, then maybe we should reverse my decision to combine their functionality and add __atomic_test_and_set and __atomic_clear to the __atomic library stable... Im not fond of that since its just a subclass of exchange and store... is it their weirdness on those targets you are concerned with? I could argue that those weirdnesses are only going to show up on targets which dont have other support anyway... hmm. As I make that argument, it also occurs to me how it can break. If atomic_flag HAS to be lockfree, it possible that there is another object the same size which is not lock free, and if I try to exchange (1) or store(0) into that object, the test_and-set or clear routine would be invoked and in a lock-free way.. which is going to break that object.. sigh. OK, I think i need to revert my position and introduce __atomic_test_and_set() and __atomic_clear(). bah. I'll work on that today. btw, how did that patch pass __sync_release tests? expand_builtin_sync_lock_release calls the __atomic_store expander now with 0... oh, its probably doing a compare_and_swap loop... in any case, the sync_lock_release expander would have to be put back in place. Andrew
Re: [PATCH] Fix errors in expand_atomic_store.
On 11/02/2011 07:18 AM, Andrew MacLeod wrote: OK, I think i need to revert my position and introduce __atomic_test_and_set() and __atomic_clear(). bah. I'll work on that today. I don't think you do. We already have the __sync functions, and we should just use those. What we need is a preprocessor flag that tells you that you need to use those older sync routines. r~
Re: [PATCH] Fix errors in expand_atomic_store.
On 11/02/2011 11:25 AM, Richard Henderson wrote: On 11/02/2011 07:18 AM, Andrew MacLeod wrote: OK, I think i need to revert my position and introduce __atomic_test_and_set() and __atomic_clear(). bah. I'll work on that today. I don't think you do. We already have the __sync functions, and we should just use those. actually, that should be sufficinent. What we need is a preprocessor flag that tells you that you need to use those older sync routines. why? its only for the atomic_flag object that those routines are even used, so the C++ or C1x implementation of atomic_flag can just use the existing __sync routines. Since they are defined by the standard as being required to be lock free, I dont need to worry about them in the library which is what I was thinking. I'd just change store and exchange to not try to use those patterns, and then use the sync patterns in implementing atomic_flag. They would be documented as required for that functionality. that should work shouldnt it? Andrew
Re: [PATCH] Fix errors in expand_atomic_store.
On 11/02/2011 08:44 AM, Andrew MacLeod wrote: that should work shouldnt it? Yes, so long as the atomic_flag object only wants one memory model. Which I suppose it does for implementing a mutex. r~
Re: [PATCH] Fix errors in expand_atomic_store.
From: Richard Henderson r...@redhat.com Date: Tue, 01 Nov 2011 08:15:51 -0700 Given that I believe that essentially all Sparcs still running are actually v9 and have native CAS, I think we can ignore this problem entirely. Unfortunately, this is not true. Otherwise we could change the 32-bit default code generation to v9 from v7 under Linux.
Re: [PATCH] Fix errors in expand_atomic_store.
On 11/01/2011 01:20 PM, David Miller wrote:
Unfortunately, this is not true.
Otherwise we could change the 32-bit default code generation to
v9 from v7 under Linux.
For v7, pa-risc, and sh, we originally allowed the test_and_set and
lock_release patterns to do non-obvious things with 0/1 constants.
My proposal is to *not* carry that over to the __atomic patterns.
The PA and SH targets have already switched to use kernel helpers,
and no longer rely on this hack. The only one left is Sparc v7.
(1) Are there really live v7 still around?
At least with v8 we have SWAP, with which we can implement the full
__atomic_exchange pattern sans hackery. We can't do that with just LDSTUB.
(2) Can we have the kernel implement some {SWAP,CAS}{4,8} primitives (possibly
via a special trap) that we can export from libgcc, as we do for ARM, PA,
SH?
I believe that would allow all of the non-embedded linux to support all of
the c++11 atomic operations without having to resort to spinlocks.
r~
Re: [PATCH] Fix errors in expand_atomic_store.
From: Richard Henderson r...@redhat.com
Date: Tue, 01 Nov 2011 13:48:26 -0700
(2) Can we have the kernel implement some {SWAP,CAS}{4,8} primitives (possibly
via a special trap) that we can export from libgcc, as we do for ARM, PA,
SH?
I believe that would allow all of the non-embedded linux to support all of
the c++11 atomic operations without having to resort to spinlocks.
Yes, I was just looking into this right now. I didn't realize that PA, SH,
and ARM had added these kernel hooks to solve this problem.
Re: [PATCH] Fix errors in expand_atomic_store.
(1) Are there really live v7 still around? At least with v8 we have SWAP, with which we can implement the full __atomic_exchange pattern sans hackery. We can't do that with just LDSTUB. I think that we can drop v7 support at this point but not v8 because of Leon. -- Eric Botcazou
Re: [PATCH] Fix errors in expand_atomic_store.
On 11/01/2011 11:15 AM, Richard Henderson wrote: On 11/01/2011 04:56 AM, Andrew MacLeod wrote: well, the reason for it was so that __atomic_store can be used as a replacement for sync_lock_release on such targets... And what was your replacement for sync_test_and_set? If you don't have that pair, you don't have a replacement. store (m, 0) is release and t = exchange (m, 1) is test_and_set.
[PATCH] Fix errors in expand_atomic_store.
* optabs.c (expand_atomic_store): Use create_fixed_operand for
atomic_store optab. Don't try to fall back to sync_lock_release.
---
The create_fixed_operand thinko is obvious. The sync_lock_release is
more subtle. The target is allowed to support only storing 0/1 with
the test_and_set/lock_release pair, and it's allowed to support that
in non-obvious ways. We don't want to get involved in that.
r~
---
gcc/ChangeLog.mm |5 +
gcc/optabs.c | 21 +
2 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 1ecab53..d8ab97e 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7118,32 +7118,13 @@ expand_atomic_store (rtx mem, rtx val, enum memmodel
model)
icode = direct_optab_handler (atomic_store_optab, mode);
if (icode != CODE_FOR_nothing)
{
-
- create_output_operand (ops[0], mem, mode);
+ create_fixed_operand (ops[0], mem);
create_input_operand (ops[1], val, mode);
create_integer_operand (ops[2], model);
if (maybe_expand_insn (icode, 3, ops))
return const0_rtx;
}
- /* A store of 0 is the same as __sync_lock_release, try that. */
- if (CONST_INT_P (val) INTVAL (val) == 0)
-{
- icode = direct_optab_handler (sync_lock_release_optab, mode);
- if (icode != CODE_FOR_nothing)
- {
- create_fixed_operand (ops[0], mem);
- create_input_operand (ops[1], const0_rtx, mode);
- if (maybe_expand_insn (icode, 2, ops))
- {
- /* lock_release is only a release barrier. */
- if (model == MEMMODEL_SEQ_CST)
- expand_builtin_mem_thread_fence (model);
- return const0_rtx;
- }
- }
-}
-
/* If the size of the object is greater than word size on this target,
a default store will not be atomic, Try a mem_exchange and throw away
the result. If that doesn't work, don't do anything. */
--
1.7.6.4
