[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: docs/LanguageExtensions.rst:1998
+``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``,
+``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics.
+
----------------
rjmccall wrote:
> Thank you for adding this documentation.  Please do clarify what the memory ordering semantics actually are when the atomic object does not need to be updated, though, and verify that target code generation actually obeys that ordering.  For example, if the memory ordering makes this a release operation, `__atomic_fetch_min` must always store the result back to the atomic object, even if the new value was actually greater than the stored value; I believe that would not be required with a relaxed operation.
Okay, that's not what I was asking for.  It's okay to assume that people understand the basic memory orderings; you don't need to copy/paste generic descriptions of them here.  There is something special about min/max vs. the rest of these atomic update operations, however, which is that min and max don't always change the value of the variable.  (Technically this is true of corner cases of many of the other operations — for example, you could atomically add 0 to a variable or multiply a variable by 1 — but the basic operation of min/max makes this corner case a lot more important.)  I am just asking you to state definitively whether the atomic operation is still appropriately ordered if the object's value does not change.

If you don't understand what I'm getting at here, maybe you should just add the relaxed ordering for now.


Repository:
  rC Clang

https://reviews.llvm.org/D46386