This is an archive of the discontinued LLVM Phabricator instance.

IR: Add atomicrmw uinc_wrap and udec_wrap
ClosedPublic

Authored by arsenm on Nov 3 2022, 2:59 PM.

Details

Summary

These are essentially add/sub 1 with a clamping value.

AMDGPU has instructions for these. CUDA/HIP expose these as
atomicInc/atomicDec. Currently we use target intrinsics for these,
but those do not carry the ordering and syncscope. Add these to
atomicrmw so we can carry these and benefit from the regular
legalization processes.

Diff Detail

Event Timeline

arsenm created this revision.Nov 3 2022, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 2:59 PM
arsenm requested review of this revision.Nov 3 2022, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 2:59 PM
Herald added a subscriber: wdng. · View Herald Transcript

I'm very much in favor. One question, should it be uinc and udec, just for consistency? I guess we can simply add nvptx support afterwards too (@tra).

llvm/docs/LangRef.rst
10584

Mark the comparisons as unsigned.

llvm/include/llvm/IR/Instructions.h
773

Mark the comparisons as unsigned.

tra added a comment.Nov 3 2022, 4:26 PM

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

llvm/docs/LangRef.rst
10583

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.

10584

v -> val ?

llvm/include/llvm/IR/Instructions.h
768

Nit: "increment by one" and "decrement by one" below?

769

Source of old? In the examples above it was a parameter to minnum/maxnum. Nere it should probably be *p.

773

Unbalanced ().

arsenm added inline comments.Nov 3 2022, 4:34 PM
llvm/docs/LangRef.rst
10583

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.

I've only looked at the LangRef and header, but the direction makes sense to me.

llvm/docs/LangRef.rst
10584

Should be *ptr - 1

llvm/include/llvm/IR/Instructions.h
769

It's fine, old is used consistently here.

tianshilei1992 added inline comments.Nov 4 2022, 5:22 AM
llvm/lib/Transforms/Utils/LowerAtomic.cpp
44

nit: update the header as well

82

0 -> 1?

85

is ConstantInt::getNullValue better here?

foad added a subscriber: foad.Nov 5 2022, 4:30 AM
foad added inline comments.
llvm/docs/LangRef.rst
10584

Parentheses are mismatched

arsenm updated this revision to Diff 473437.Nov 5 2022, 9:38 AM
arsenm marked 9 inline comments as done.
arsenm retitled this revision from IR: Add atomicrmw inc and dec to IR: Add atomicrmw uinc_wrap and udec_wrap.

Rename and address comments

llvm/lib/Transforms/Utils/LowerAtomic.cpp
85

That's really Constant::getNullValue which dispatches over the type and just gets back to this for ConstantInts

JonChesterfield edited the summary of this revision. (Show Details)Nov 7 2022, 3:32 AM
JonChesterfield added inline comments.
llvm/docs/LangRef.rst
10586

The expressions here look suspicious - I thought these operated as inc/dec then modulo value. Does inc(p,10) really go to zero if p has the initial value 15? If so clamp might be a better name.

Likewise the ==0 in dec, though I think unsigned compare makes that expression work. Is it *ptr = ((*ptr -1) % val) ?

arsenm added inline comments.Nov 7 2022, 12:17 PM
llvm/docs/LangRef.rst
10586

https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#atomicinc

The description here also matches the ISA documentation. I also interpreted this as a clamp, but dec confused me

aeubanks added inline comments.
llvm/docs/ReleaseNotes.rst
107

remove indentation? this is under the "constant expression" section right now

ruiling added a subscriber: ruiling.Nov 7 2022, 4:51 PM
ruiling added inline comments.
llvm/docs/LangRef.rst
10586

The two operations work like maintaining the pointer to a circular/ring buffer (with val+1 elements). The uinc_wrap would move the pointer forward while the udec_wrap would move the pointer backward. IMO, this is more of a wrap-around than clamp.

arsenm updated this revision to Diff 486619.Jan 5 2023, 10:16 AM

Rebase, lot of opaque pointer test churn.

Fix release note indentation, and add textual description of operation to LangRef

foad added inline comments.Jan 5 2023, 11:09 PM
llvm/docs/LangRef.rst
10583

I think "when incremented past input value" or "when incremented above input value" would be clearer. If val is 8 then *ptr can take on values 0..8, not just 0..7.

arsenm added inline comments.Jan 6 2023, 5:11 AM
llvm/docs/LangRef.rst
10583

I copied this description ~verbatim from the documentation for the instruction

arsenm updated this revision to Diff 489060.Jan 13 2023, 10:34 AM

Adjust langref description, expand and test in the targets where it was easy

jrtc27 added inline comments.Jan 13 2023, 10:45 AM
llvm/test/CodeGen/RISCV/atomicrmw-uinc-udec-wrap.ll
2

Atomics need coverage with the A extension too

8

These x86 CHECK lines need deleting

arsenm added inline comments.Jan 13 2023, 11:00 AM
llvm/test/CodeGen/RISCV/atomicrmw-uinc-udec-wrap.ll
2

I thought it was suspicious I didn't need to touch shouldExpandAtomicRMWInIR

arsenm updated this revision to Diff 489075.Jan 13 2023, 11:19 AM

Fix RISCV, some clang-format

lkail added a subscriber: lkail.Jan 13 2023, 1:13 PM
asb added a comment.Jan 17 2023, 5:52 AM

RISC-V changes LGTM.

nikic accepted this revision.Jan 24 2023, 12:58 AM
nikic added a subscriber: nikic.

Trusting the experts that these are indeed the generally accepted semantics for these operations, the implementation LGTM.

llvm/docs/ReleaseNotes.rst
111

Missing period

This revision is now accepted and ready to land.Jan 24 2023, 12:58 AM
foad added inline comments.Jan 25 2023, 2:38 AM
llvm/docs/LangRef.rst
10584

Weird extra spaces in - 1. This is visible in the rendered HTML at https://llvm.org/docs/LangRef.html.

arsenm marked an inline comment as done.Jan 25 2023, 11:32 AM