Add design doc for atomic<T> type. #5101

csyonghe · 2024-09-18T05:54:04Z

Closes #5100.

jkwak-work

I loved the writing in the "background" section.
At the end of the section, I was convinced that we should have atomic.

jkwak-work · 2024-09-18T20:54:40Z

docs/proposals/003-atomic-t.md

+extension<T> Atomic<T>
+    where T : IAtomicable
+    where T : __BuiltinIntegerType
+{
+    [ref] T atomicAnd(T value); // returns original value
+    [ref] T atomicOr(T value); // returns original value
+    [ref] T atomicXor(T value); // returns original value
+}


I think we can add atomicIncrement and atomicDecrement for integer types.

Increment/decrement are interesting in that we can provide them via an extension by just doing an add/sub of 1, but we might in principle want allow targets to directly generate a specific increment/decrement op if they support one.

jkwak-work · 2024-09-18T20:57:11Z

docs/proposals/003-atomic-t.md

+extension int : IAtomicable {}
+extension uint : IAtomicable {}


We may want to add 64-bit integer since SM 6.6 supports it.

tangent-vector

This looks great as a starting point. None of my comments should be taken as a sign that we shouldn't check this in.

One big topic that we'll want to broach in this document (but are unlikely to make any definitive headway on) is: what is Slang's memory model?

Our choice to introduce Atomic<T> is motivated by a desire to start taking memory models seriously, but it's kind of obvious in this document that we aren't making clear statements about how atomic and non-atomic accesses interact, etc. We don't currently spell out the consequences, from the standpoint of Slang's language semantics, for aliasing the same memory as both a RWStructuredBuffer<int> and a RWStructuredBuffer<Atomic<int>> (well, we don't specify any consequences for aliasing of resources in general...).

The hard work on that front needs to happen, but not in this proposal. Still, it would be good for the proposal to note that the hard work still needs to be done for efforts like this to be truly meaningful.

tangent-vector · 2024-09-18T21:29:03Z

docs/proposals/003-atomic-t.md

+
+We define an `Atomic<T>` type that functions as a wrapper of `T` and provides atomic operations:
+```
+interface IAtomicable {}


This is not a problem to solve with this proposal, but in the long run we really ought to stop using empty interfaces for stuff like this, and then relying on intrinsics that are constrained on the interface.

Ideally, built-in interfaces like this should actually define their requirements explicitly, like:

interface IAtomicable { This atomicLoad( ref Atomic<This> location ); T atomicExchange( ref Atomic<This> location, This newValue ); // ... }

We would then make the Atomic<T> type have entirely concrete method definitions (with hints to ensure they get inlined whenever possible), and the conformances for concrete types like int and uint would then define the required operations explicitly (whether as intrinsics, or using target_switch, etc.).

Such an approach would not only be more "correct," but it also opens the door to having certain types conform in non-intrinsic ways, or even allowing user-defined conformances (e.g., to allow a user-defined type to opt into Atomic<T> support if its in-memory layout is bit-identical to a built-in atomic type).

tangent-vector · 2024-09-18T21:29:37Z

docs/proposals/003-atomic-t.md

+extension float : IAtomicable {}
+extension half : IAtomicable {}
+
+struct Atomic<T>


Why isn't there a constraint of T : IAtomicable here?

tangent-vector · 2024-09-18T21:32:41Z

docs/proposals/003-atomic-t.md

+    [ref] T compareExchange(T compareValue, T newValue); // returns old value.
+    [ref] T atomicAdd(T value); // returns original value
+    [ref] T atomicSub(T value); // returns original value
+    [ref] T atomicMax(T value); // returns original value
+    [ref] T atomicMin(T value); // returns original value


In principle, the only truly universal operations that an atomicable type would need to support are load/store and exchange. Compare-exchange relies on the type being comparable (and I'd need to double-check how comparisons are done for floating-point atomic compare-exchange), and the add/sub/min/max operations rely on the type supporting the relevant mathematical operations.

It seems like we might actually need a hierarchy of atomic-related interfaces, representing specific subsets of the available functionality.

Added IAtomicable hierarchy.

tangent-vector · 2024-09-18T21:40:09Z

docs/proposals/003-atomic-t.md

+extension<T> Atomic<T>
+    where T : IAtomicable
+    where T : __BuiltinIntegerType


I get what this is doing, but I also worry a little bit, when I think about how this would translate to my "move the required operations into the interface" approach. Effectively, this is saying that any type that conforms IAtomicable & __BuiltinIntegerType has additional requirements beyond those that are stated for either interface alone.

I really do think the right answer here will be a hierarchy of interfaces, along the lines of:

interface IAtomicable { ... } // load/store and exchange interface IAtomicCompareExchangeable : IAtomicable { ... } // compare-exchange interface IAtomicNumeric : IAtomicCompareExchangeable { ... } // add/sub/min/max interface IAtomicLogical : IAtomicCompareExchangeable { ... } // and/or/xor typealias IAtomicInteger = IAtomicNumeric & IAtomicLogical;

(TBD whether add/sub and min/max should be separated from one another)

(Note: I did not make IAtomicLogical inherit from IAtomicNumeric because it is in principle possible to support Atomic<bool>, which would have logical operations but not add/sub/min/max)

It is likely to be rare for programmers to define their own generics that work with Atomic<T> for various T, so putting the burden on them to specify the range of atomic operations they need to be able to perform doesn't seem like too much.

That said, it is clear that my little hierarchy above closely mirrors the kind of hierarchy we need for builtin scalar types, so it is potentially frustrating to have both IWhatever and IAtomicWhatever as distinct interfaces.

Used hierarchy of IAtomic to clean this up.

tangent-vector · 2024-09-18T21:41:00Z

docs/proposals/003-atomic-t.md

+extension<T> Atomic<T>
+    where T : IAtomicable
+    where T : __BuiltinIntegerType
+{
+    [ref] T atomicAnd(T value); // returns original value
+    [ref] T atomicOr(T value); // returns original value
+    [ref] T atomicXor(T value); // returns original value
+}


Increment/decrement are interesting in that we can provide them via an extension by just doing an add/sub of 1, but we might in principle want allow targets to directly generate a specific increment/decrement op if they support one.

tangent-vector · 2024-09-18T21:41:27Z

docs/proposals/003-atomic-t.md

+We allow `Atomic<T>` to be defined anywhere: as struct fields, as array elements, as elements of `RWStructuredBuffer` types,
+or as groupshared variable types. For example, in global memory:


This conspicuously doesn't list local variables, function parameters, etc.

Added mention of local/global variable and function parameter here.

csyonghe · 2024-09-19T17:35:58Z

@tangent-vector I updated the doc to reflect the review suggestions.

I am hesitant about moving the requirement into the interfaces, not because they are not better, but because they introduce more implementation complexity than what it worth. Currently, we only plan for T in Atomic<T> to be builtin data types and not any user-defined data types. In this case, all atomic operations are going to map to slang IR opcodes. Doing it per-data-type as implementation to IAtomic means we need to duplicate a lot of the intrinsic_op implementation across all basic types, versus the empty interface implementation allows us to define them in a single centralized place for simplicity.

jeffbolznv · 2024-09-19T20:47:57Z

One big topic that we'll want to broach in this document (but are unlikely to make any definitive headway on) is: what is Slang's memory model?

IMO, at the very least, this doc should say that these are relaxed atomics and give a rough definition of what that means (no memory ordering is enforced by the instruction).

jeffbolznv · 2024-09-19T20:49:15Z

docs/proposals/003-atomic-t.md

+struct Atomic<T : IAtomicable>
+{
+    T load();
+    [ref] void store(T newValue); // Question: do we really need this?


Yes, you surely want this, atomic stores can often be lowered to a much cheaper hardware instruction than whatever you would replace it with (e.g. atomic exchange).

jeffbolznv · 2024-09-19T20:51:22Z

docs/proposals/003-atomic-t.md

+
+struct Atomic<T : IAtomicable>
+{
+    T load();


Do you intend to support syntactic sugar where you can use the Atomic as an lvalue/rvalue and assign without using load/store? This is pretty common.

Yes we will need that.

jeffbolznv · 2024-09-19T20:52:56Z

docs/proposals/003-atomic-t.md

+    T load();
+    [ref] void store(T newValue); // Question: do we really need this?
+    [ref] T exchange(T newValue); // returns old value
+    [ref] T compareExchange(T compareValue, T newValue); // returns old value.


There are sometimes two forms of compareExchange (strong vs weak). Should say which this is.

According to SPIRV spec, the OpAtomicCompareExchangeWeak is deprecated and we only have OpAtomicCompareExchange, so perhaps it is fine to just assume it means weak here?

csyonghe · 2024-09-20T01:14:06Z

@jeffbolznv The design is updated in PR https://github.com/shader-slang/slang/pull/5125/files
We now have explicit MemoryOrder parameter to specify whether or not the semantics is relaxed or SeqCst.
We can consider adding other enum cases that spirv exposes too.

Add design doc for atomic<T> type.

1deb3f7

csyonghe requested a review from tangent-vector September 18, 2024 05:54

jkwak-work previously approved these changes Sep 18, 2024

View reviewed changes

tangent-vector previously approved these changes Sep 18, 2024

View reviewed changes

Update design doc.

26b7233

csyonghe dismissed stale reviews from tangent-vector and jkwak-work via 26b7233 September 19, 2024 17:25

csyonghe added 3 commits September 19, 2024 10:25

Merge remote-tracking branch 'official/master' into atomic_t

ae9c0f4

Fix.

4b6e1ea

More comment on parameters.

b878c80

tangent-vector approved these changes Sep 19, 2024

View reviewed changes

csyonghe merged commit 1560326 into shader-slang:master Sep 19, 2024

jeffbolznv reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add design doc for atomic<T> type. #5101

Add design doc for atomic<T> type. #5101

csyonghe commented Sep 18, 2024 •

edited

Loading

jkwak-work left a comment

jkwak-work Sep 18, 2024

tangent-vector Sep 18, 2024

jkwak-work Sep 18, 2024

tangent-vector left a comment

tangent-vector Sep 18, 2024

tangent-vector Sep 18, 2024

csyonghe Sep 19, 2024

tangent-vector Sep 18, 2024

csyonghe Sep 19, 2024

tangent-vector Sep 18, 2024

csyonghe Sep 19, 2024

tangent-vector Sep 18, 2024

tangent-vector Sep 18, 2024

csyonghe Sep 19, 2024 •

edited

Loading

csyonghe commented Sep 19, 2024

jeffbolznv commented Sep 19, 2024

jeffbolznv Sep 19, 2024

jeffbolznv Sep 19, 2024

csyonghe Sep 20, 2024

jeffbolznv Sep 19, 2024

csyonghe Sep 20, 2024

csyonghe commented Sep 20, 2024

		extension int : IAtomicable {}
		extension uint : IAtomicable {}

		We allow `Atomic<T>` to be defined anywhere: as struct fields, as array elements, as elements of `RWStructuredBuffer` types,
		or as groupshared variable types. For example, in global memory:

Add design doc for atomic<T> type. #5101

Add design doc for atomic<T> type. #5101

Conversation

csyonghe commented Sep 18, 2024 • edited Loading

jkwak-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangent-vector left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csyonghe Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

csyonghe commented Sep 19, 2024

jeffbolznv commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csyonghe commented Sep 20, 2024

csyonghe commented Sep 18, 2024 •

edited

Loading

csyonghe Sep 19, 2024 •

edited

Loading