Initial implementation of SP#015 `DescriptorHandle<T>`. #6028

csyonghe · 2025-01-08T01:26:32Z

Closes #6038.

Closes #6010.

csyonghe · 2025-01-08T22:24:39Z

/format

slangbot · 2025-01-08T22:25:25Z

🌈 Formatted, please merge the changes from this PR

Format code for PR shader-slang#6028

natevm · 2025-01-09T08:09:55Z

Just looked over the proposal. LGTM 👍🏻

jkwak-work

Looks good to me.

I left some questions, but I probably have some misunderstandings.

docs/proposals/015-descriptor-handle.md

source/slang/hlsl.meta.slang

jkwak-work

Looks good to me.

docs/user-guide/03-convenience-features.md

jkwak-work

Looks good to me.

tangent-vector · 2025-01-09T22:38:33Z

docs/proposals/015-descriptor-handle.md

+    where T : IOpaqueDescriptor
+{
+    [require(hlsl_glsl_spirv)]
+    __init(uint2 value); // For HLSL, GLSL and SPIRV targets only.


I'm guessing this constructor also requires a certain shader model version of SPIR-V capability.

Another note is that I'd expect that operation to be de facto available for CPU and CUDA targets as well, since the representation of a handle on those platforms is "just bits" and should fit in 64 bits.

It would be nice to have a convenience constructor that takes a UInt64 on targets that support such a type. That could be directly implemented on targets that can support it that way, and can decompose the 64-bit integer into a uint2 on the others.

On CPU and CUDA, a StructuredBuffer is actually 16 bytes, storing both a pointer and a size to support the GetDimension operation, so we can't actually support this constructor on CPU/CUDA for all resource types, unfortunately.

tangent-vector · 2025-01-09T22:42:08Z

docs/proposals/015-descriptor-handle.md

+}
+```
+
+All builtin types that implements `IOpaqueDescriptor` interface provide a convenience typealias for `DescriptorHandle<T>`. For example, `Texture2D.Handle` is an alias for `DescriptorHandle<Texture2D>`.


Not a comment on this proposal, but I wonder if that behavior could be provided automatically via an extension on types that conform to IOpaqueDescriptor along the lines of:

extension<T : IOpaqueDescriptor> T { typealias Handle = DescriptorHandle<T>; }

Yes, this is originally implemented this way but I hit a bug in the type system. I need to fix the type system first before this is possible.

tangent-vector · 2025-01-09T22:44:22Z

docs/proposals/015-descriptor-handle.md

+
+- `operator *` to deference the pointer and obatin the actual descriptor handle `T`.
+- Implicit conversion to `T` when used in a location that expects `T`.
+- When targeting HLSL, GLSL and SPIRV, `DescriptorHandle<T>` can be casted to and from a `uint2` value.


Should we be explicit that this only supports explicit casts, and not implicit type conversions?

In my own usage, I try to distinguish (explicit) casting from (implicit) conversion of types, but the distinction might be too subtle to rely on somebody reading this document to know off-hand (and there are also people who apply those terms differently).

tangent-vector · 2025-01-09T22:45:12Z

docs/proposals/015-descriptor-handle.md

+- `operator *` to deference the pointer and obatin the actual descriptor handle `T`.
+- Implicit conversion to `T` when used in a location that expects `T`.
+- When targeting HLSL, GLSL and SPIRV, `DescriptorHandle<T>` can be casted to and from a `uint2` value.
+- Equality comparison.


I assume this implicitly means equality and inequality.

If we end up adding some kind of IHashable interface to the core library, then we'd also want descriptor handles to be hashable.

tangent-vector · 2025-01-09T22:45:59Z

docs/proposals/015-descriptor-handle.md

+    // Alternatively, the following syntax is also allowed, to
+    // make `DescriptorHandle` appear more like a pointer:
+    t->Sample(*sampler, float2(0, 0));


Unrelated to this proposal, but I find it distasteful that we are adding more and more C++-isms to our language.

Ack, but not something we can do here since this was already added to the language along with the pointer support.

tangent-vector · 2025-01-09T22:57:35Z

docs/proposals/015-descriptor-handle.md

+If the descriptor handle value is not uniform and `nonuniform` is not called, the result may be
+undefined.
+
+### Combind Texture Samplers


Typo: "Combind" -> "Combined"

I don't expect the following to change in this document, but in order to raise a discussion point:

The term "combined texture sampler" is one I find unfortunate, because a "texture sampler" superficially sounds like a thing that samples textures, rather than a pair of two things: a texture and a sampler. Adding the adjective "combined" doesn't do a whole lot to dissuade somebody from that interpretation.

When possible, I tend to refer to such things as "combined texture-sampler pairs." A "texture-sampler pair" clearly communicates that it is (semantically) a pair, and what it is a pair of: a texture and a sampler. In this context the "combined" prefix makes it clear that while the thing is semantically a pair, something might be done as part of combining them, so that the resulting combination could be different from what would result from forming such a pair type in user code.

tangent-vector · 2025-01-09T23:06:07Z

docs/proposals/015-descriptor-handle.md

+
+### Combind Texture Samplers
+
+On platforms without native support for combined texture samplers, we will use both components of the


Ah... this part of things makes subtle use of the fact that the (current) D3D case only needs 32 bits to represent a handle, so that a uint2 is actually sufficient for storing two handles (a texture handle and a sampler handle).

This choice would break down the line if handles on D3D eventually become 64-bit values. All of the descriptor types except combined texture-sampler pairs would be able to adapt (using all the bits of the uint2), but combined texture-sampler pairs would require changes to the DescriptorHandle<> type given here.

This is true, and in that case we will need to change the size of DescriptorHandle<Sampler2D> on D3D. Note that with this revised proposal, DescriptorHandle size is target dependent and we are allowed to make it having different size based on the underlying descriptor type.

docs/proposals/015-descriptor-handle.md

source/slang/hlsl.meta.slang

tangent-vector · 2025-01-09T23:11:49Z

source/slang/slang-ir-glsl-legalize.cpp

+    // At this stage, we can safely remove the generic `getDescriptorFromHandle` function
+    // despite it being marked `export`.
+    for (auto inst : module->getGlobalInsts())
+    {
+        if (auto genFunc = as<IRGeneric>(inst))
+        {
+            if (!genFunc->hasUses())
+            {
+                toRemove.add(genFunc);
+            }
+        }
+    }


Is there a risk that this logic could remove IR generics that it shouldn't, in some future scenario?

I suppose that we don't anticipate any of our targets supporting actual generics in binary code (only specialized or dynamic-dispatch-ified versions).

I agree there is risk, and we would have to deal with this issue in the future.

…#6028) * Initial implementation of `ResourcePtr<T>`. * Update docs * Fix build error. * Add more discussion. * Update documentation. * Update TOC. * Fix. * Fix. * Add test case for custom `getResourceFromBindlessHandle`. * Add namehint to generated descriptor heap param. * Fix. * Fix. * format code * Rename to `DescriptorHandle`, and add `T.Handle` alias. * Fix compiler error. * Fix. * Fix build. * Renames. * Fix documentation. * Documentation fix. --------- Co-authored-by: slangbot <[email protected]>

Initial implementation of ResourcePtr<T>.

826e6eb

csyonghe added the pr: non-breaking PRs without breaking changes label Jan 8, 2025

csyonghe requested review from tangent-vector, expipiplus1 and a team as code owners January 8, 2025 01:26

csyonghe added 12 commits January 7, 2025 17:52

Update docs

1f35e7e

Fix build error.

b531432

Add more discussion.

f52c801

Update documentation.

5d719c4

Update TOC.

3c160cf

Fix.

8d789b2

Fix.

259adfd

Add test case for custom getResourceFromBindlessHandle.

4ca71e1

Add namehint to generated descriptor heap param.

bc4e441

Fix.

e6f2328

Fix.

17477e8

Merge conflicts.

c0371d7

format code

e0f5d9c

slangbot mentioned this pull request Jan 8, 2025

Format code for PR #6028 csyonghe/slang#15

Merged

csyonghe added 2 commits January 8, 2025 14:25

Merge pull request #15 from slangbot/format-6028-bindless

6621a99

Format code for PR shader-slang#6028

Rename to DescriptorHandle, and add T.Handle alias.

2378c02

csyonghe changed the title ~~Initial implementation of ResourcePtr<T>.~~ Initial implementation of SP#016 DescriptorHandle<T>. Jan 8, 2025

csyonghe changed the title ~~Initial implementation of SP#016 DescriptorHandle<T>.~~ Initial implementation of SP#015 DescriptorHandle<T>. Jan 9, 2025

csyonghe added 3 commits January 8, 2025 16:02

Fix compiler error.

210b08f

Fix.

bea013c

Fix build.

88993a1

jkwak-work previously approved these changes Jan 9, 2025

View reviewed changes

docs/proposals/015-descriptor-handle.md Outdated Show resolved Hide resolved

docs/proposals/015-descriptor-handle.md Outdated Show resolved Hide resolved

source/slang/hlsl.meta.slang Outdated Show resolved Hide resolved

csyonghe added 2 commits January 9, 2025 14:18

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

b994a2f

Renames.

41e99e4

csyonghe dismissed jkwak-work’s stale review via 41e99e4 January 9, 2025 22:21

jkwak-work previously approved these changes Jan 9, 2025

View reviewed changes

docs/user-guide/03-convenience-features.md Outdated Show resolved Hide resolved

Fix documentation.

defd7ae

csyonghe dismissed jkwak-work’s stale review via defd7ae January 9, 2025 22:49

jkwak-work previously approved these changes Jan 9, 2025

View reviewed changes

tangent-vector previously approved these changes Jan 10, 2025

View reviewed changes

Documentation fix.

0666eb5

csyonghe dismissed stale reviews from tangent-vector and jkwak-work via 0666eb5 January 10, 2025 18:56

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

788f59f

csyonghe merged commit 5290c58 into shader-slang:master Jan 10, 2025
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of SP#015 `DescriptorHandle<T>`. #6028

Initial implementation of SP#015 `DescriptorHandle<T>`. #6028

csyonghe commented Jan 8, 2025 •

edited

Loading

csyonghe commented Jan 8, 2025

slangbot commented Jan 8, 2025

natevm commented Jan 9, 2025

jkwak-work left a comment

jkwak-work left a comment

jkwak-work left a comment

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

csyonghe Jan 10, 2025

tangent-vector Jan 9, 2025

csyonghe Jan 10, 2025

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

csyonghe Jan 10, 2025

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

tangent-vector Jan 9, 2025

csyonghe Jan 10, 2025

tangent-vector Jan 9, 2025

csyonghe Jan 10, 2025


		### Combind Texture Samplers

		On platforms without native support for combined texture samplers, we will use both components of the

Initial implementation of SP#015 DescriptorHandle<T>. #6028

Initial implementation of SP#015 DescriptorHandle<T>. #6028

Conversation

csyonghe commented Jan 8, 2025 • edited Loading

csyonghe commented Jan 8, 2025

slangbot commented Jan 8, 2025

natevm commented Jan 9, 2025

jkwak-work left a comment

Choose a reason for hiding this comment

jkwak-work left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Initial implementation of SP#015 `DescriptorHandle<T>`. #6028

Initial implementation of SP#015 `DescriptorHandle<T>`. #6028

csyonghe commented Jan 8, 2025 •

edited

Loading