dash

dash copied to clipboard

Our AtomicTest.cc test suite has two confusing unit tests:

• AtomicTest.PunnedType
• AtomicTest.PunnedFetchOp

The thing is that the used elem_t for dash::Atomic is a container with two value members. This does not satisfy the is_atomic_compatible trait. If we add the following line to both tests it cannot compile:

  static_assert(dash::is_atomic_compatible<value_t>::value, "invalid type");

So what was the original purpose for this? It does not really make sense since this value type cannot be atomic, neither with MPI or in hardware.

You can still atomically read/write/cas structs for which sizeof(T) <= 8B, just not perform arithmetic operations on them. The restrictions are encoded in GlobAtomicRef::set etc.

The whole dash::atomic stuff is still experimental and subject to change.

@devreal: I see your point, but the problem is that sizeof(struct) (without packed) depends also on the compiler.

@rkowalewski: The is_atomic_compatible trait is too simple. Instead it should return tags, denoting what atomic operations are supported.

@devreal: I see your point, but the problem is that sizeof(struct) (without packed) depends also on the compiler.

Yes. That is why we check for sizeof(T) to be smaller than 8 in dash::dart_punned_datatype. If the struct is larger than that (potentially due to padding) we cannot use MPI to atomically get/set/cas the values. I don't think padding varies between compilers (on the same architecture).

@rkowalewski: The is_atomic_compatible trait is too simple. Instead it should return tags, denoting what atomic operations are supported.

We static_assert for the kind of operation in GlobAtomicRef::op etc for this. A type may be set/get-able but not be used in arithmetic operations (like punned structs). CAS is only available for integral and punned types but not for floating point. Still floating point and punned types can be used in dash::Atomic and having this as static_assert gives users halfway decent error messages. In essence, I would not change is_atomic_compatible.

Still an issue here?