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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Nice. NVPTX will be able to benefit from this, too as it also has `.inc` and `.dec` atomics.



================
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)``
----------------
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.


================
Comment at: llvm/docs/LangRef.rst:10524
+-  inc: ``*ptr = (old >= val) ? 0 : (*ptr + 1)``
+-  dec: ``*ptr = (*ptr == 0) || (*ptr > v)) ? val : (val - 1)``
 
----------------
`v` -> `val` ?


================
Comment at: llvm/include/llvm/IR/Instructions.h:765
 
+    /// Increment one up to a maximum value.
+    /// *p = (old >= v) ? 0 : (old + 1)
----------------
Nit: "increment by one"  and "decrement by one" below?


================
Comment at: llvm/include/llvm/IR/Instructions.h:766
+    /// Increment one up to a maximum value.
+    /// *p = (old >= v) ? 0 : (old + 1)
+    Inc,
----------------
Source of `old`? In the examples above it was a parameter to `minnum/maxnum`. Nere it should probably be `*p`.


================
Comment at: llvm/include/llvm/IR/Instructions.h:770
+    /// Decrement one until a minimum value or zero.
+    /// *p = (old == 0) || (old > v)) ? v : (v - 1)
+    Dec,
----------------
jdoerfert wrote:
> Mark the comparisons as unsigned.
Unbalanced `()`.


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

https://reviews.llvm.org/D137361