[PATCH] D137361: IR: Add atomicrmw inc and dec

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/LangRef.rst:10523
 -  fmin: ``*ptr = minnum(*ptr, val)`` (match the `llvm.minnum.*`` intrinsic)
+-  inc: ``*ptr = (old >= val) ? 0 : (*ptr + 1)``
+-  dec: ``*ptr = (*ptr == 0) || (*ptr > v)) ? val : (val - 1)``
----------------
tra wrote:
> Where does `old` come from? Did you mean `*ptr` ?
> 
> I think the meaning of `val` should also be documented. AFAICT, it's the maximum allowed values in `*p` and the inc/dec are expected to wrap resulting values within the range `[0, val]`.
> 
> My naive expectation was that we'd want to increment/decrement by `val`, but this `inc/dec within [0..val]` behavior matches what NVPTX instructions do. I'm not sure If it's an established convention that I've been lucky to miss till now or if we're simply encoding what existing hardware does. If it's the latter, is it universal/useful enough to add to IR? 
> 
> The answer is probably 'yes', as we already seem to have two back-ends that could benefit, but it would be great to hear from folks familiar with other back-ends.
Yes, old should be *ptr. The increment/decrement is always 1. If it was just add/sub 1, there wouldn't' be a need for a separate operation.

A few years ago I had a patch to instead allow attaching memory orderings and scope to arbitrary calls, but that feels way overengineered to handle these 2 cases. I see no reason to not just add any atomic anyone's put into any hardware or language in atomicrmw. It's easy enough to expand anything for targets that don't support it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137361/new/

https://reviews.llvm.org/D137361