Closed devreal closed 1 week ago
Not sure the two step replacement is still 'atomic', dependent upon what assembly is generated with the double evaluation.
We abused volatile because there was no good C99 way of saying its an atomic variable and the compiler shouldn't reorder around it, even if we are not doing fetch_and_op on it. In C11 there are all the good types we need, and requiring c11 is not unreasonable nowadays.
There is a difference between ((volatile int)v)++ and ((atomic int)a)++: the later will substitute for a fetch_and_add automatically, the former will only enforce that no reordering and register reload is done but will use a normal ADD assembly. That may have performance implications, whether that matters is to be seen.
Not sure the two step replacement is still 'atomic', dependent upon what assembly is generated with the double evaluation.
The current version is not atomic either. I assume it doesn't have to be because it is used in parsec_list_nolock_pop_back
. I read that as assuming that there is a lock taken somewhere by the caller...
We abused volatile because there was no good C99 way of saying its an atomic variable and the compiler shouldn't reorder around it, even if we are not doing fetch_and_op on it. In C11 there are all the good types we need, and requiring c11 is not unreasonable nowadays.
Agreed. BUT: since PaRSEC interfaces with C++ (TTG) and those headers are used publicly the _Atomic
prefix is not compatible with C++ :/
Accepting a = a - 1
while refusing a--
sounds pretty random from the C++ compiler.
refcount
shall not be atomic, because it is illegal to add or remove simultaneously an item to and from multiple lists. _Atomic
would be an overkill.Anyway, the following patch should make the C++ compiler quiet.
diff --git a/parsec/class/list_item.h b/parsec/class/list_item.h
index 0e82ac12f..cc94780f2 100644
--- a/parsec/class/list_item.h
+++ b/parsec/class/list_item.h
@@ -109,7 +109,7 @@ parsec_list_item_ring( parsec_list_item_t* first, parsec_list_item_t* last )
parsec_list_item_t* item = first;
do {
assert( item->belong_to == first->belong_to );
- item->refcount--;
+ item->refcount = item->refcount - 1;
assert( 0 == item->refcount );
item = (parsec_list_item_t*)item->list_next;
} while(item != first);
@@ -203,7 +203,7 @@ parsec_list_item_ring_chop( parsec_list_item_t* item )
item->list_prev->list_next = item->list_next;
item->list_next->list_prev = item->list_prev;
#if defined(PARSEC_DEBUG_PARANOID)
- if(item->refcount) item->refcount--;
+ if(item->refcount) item->refcount = item->refcount - 1;
item->list_prev = (parsec_list_item_t*)(void*)0xdeadbeefL;
item->list_next = (parsec_list_item_t*)(void*)0xdeadbeefL;
#endif
@@ -285,7 +285,7 @@ parsec_list_item_ring_push_sorted( parsec_list_item_t* ring,
#define PARSEC_ITEM_ATTACH(LIST, ITEM) \
do { \
parsec_list_item_t *_item_ = (ITEM); \
- _item_->refcount++; \
+ _item_->refcount = _item_->refcount + 1; \
assert( 1 == _item_->refcount ); \
_item_->belong_to = (LIST); \
} while(0)
@@ -309,7 +309,7 @@ parsec_list_item_ring_push_sorted( parsec_list_item_t* ring,
assert( _item->belong_to != (void*)_item ); \
_item->list_prev = (parsec_list_item_t*)(void*)0xdeadbeefL; \
_item->list_next = (parsec_list_item_t*)(void*)0xdeadbeefL; \
- _item->refcount--; \
+ _item->refcount = _item->refcount - 1; \
assert( 0 == _item->refcount ); \
} while (0)
#else
Do we want to promote this to v4.0 since we have code ready to apply to the problem?
Describe the bug
Building TTG with GCC 11.4.0 raises a bunch of warnings from PaRSEC:
C++20 has deprecated compound operations on
volatile
variables.A fix is to change
into
Arguably, it's only C++ that deprecated these operations and C apparently has not (yet). However, PaRSEC is now consumed by C++ projects. In the greater scheme of things,
refcount
is probably an atomic variable and should be marked as such (instead ofvolatile
).