KhronosGroup / GLSL

GLSL Shading Language Specification and Extensions
Other
337 stars 98 forks source link

Added first draft of GLSL_EXT_spirv_intrinsics #157

Closed Tobski closed 2 months ago

Tobski commented 3 years ago

This extension enables spirv opcodes, types, decorations, and storage classes to be directly encoded into GLSL via new qualifiers.

Example "headers" are included which show how the extension can be used. An initial PR for review against glslang will be available for review shortly, and I'll link that from here.

Tobski commented 3 years ago

Given that the implementation for this has now landed, can we merge this? I hadn't noticed until now. Ping @gnl21

dgkoch commented 2 years ago

@Tobski @gnl21 can we get this spec completed and merged at some point?

Tobski commented 2 years ago

@Tobski @gnl21 can we get this spec completed and merged at some point?

Yea I'd like to move this forward - waiting on some responses from @gnl21 above.

gnl21 commented 2 years ago

Sorry, looks like I had left my last comments unpublished. Just let me catch back up with what I had written ...

dgkoch commented 1 year ago

semi-annual poke..

r-potter commented 1 year ago

@Tobski What's the status on this? I just saw a comment online describing SPIR-V intrinsics as a draft specification, which was somewhat surprising (but apparently true based on this MR?)

forenoonwatch commented 1 year ago

How does this extension propose to handle nested type definitions?

#define Image2DR32UI spirv_type(id = 25, 321, 1, 0, 0, 0, 2, 33)
layout (set = 0, binding = 0) uniform Image2DR32UI u_image;

Which compiles to the following:

%6 = OpTypeImage %321 2D 0 0 0 2 R32ui

With the current syntax definition, there is no way to define the OpTypeInt which this image would depend on.

Tobski commented 1 year ago

How does this extension propose to handle nested type definitions?

#define Image2DR32UI spirv_type(id = 25, 321, 1, 0, 0, 0, 2, 33)
layout (set = 0, binding = 0) uniform Image2DR32UI u_image;

Which compiles to the following:

%6 = OpTypeImage %321 2D 0 0 0 2 R32ui

With the current syntax definition, there is no way to define the OpTypeInt which this image would depend on.

@forenoonwatch at the moment it doesn't propose anything - there's no current way to do a typedef in glslang so adding functionality to do this would be quite messy. We made the choice to defer that to a follow up extension if it was desired. I thought we had an issue about this but we don't, so I'll add one.

forenoonwatch commented 1 year ago

@forenoonwatch at the moment it doesn't propose anything - there's no current way to do a typedef in glslang so adding functionality to do this would be quite messy. We made the choice to defer that to a follow up extension if it was desired. I thought we had an issue about this but we don't, so I'll add one.

Would it not be possible to add functionality to interpret any type passed into spirv_type as the ID of its corresponding OpType instruction? I'm not sure how typedefs play into this. e.g.

#define Image2DR32UI spirv_type(id = 25, uint, 1, 0, 0, 0, 2, 33)
layout (set = 0, binding = 0) uniform Image2DR32UI u_image;

becomes

%uint = OpTypeInt 32 0
%6 = OpTypeImage %uint 2D 0 0 0 2 R32ui
Tobski commented 1 year ago

@forenoonwatch I suppose we could make an exception for existing type identifiers, little awkward but I suppose it should work fine. The main issue at this point though is that we haven't got resources to add this support to glslang... I'll ask those who did the functionality originally to see if they could do it, but I'm not expecting they'll be able to get to this any time soon (or possibly ever).

Tobski commented 1 year ago

@gnl21 I made changes along the lines of your feedback - please take a look and let me know if you still have issues.

Try commented 1 year ago

@Tobski I have a question about one corner-case in extension: what is expected to happen, if shader-developer will introduce unsupported instructions/storage-classes/etc to shader?

For example in current glslang compiler, it's possible to have task-payload in vertex shader: https://shader-playground.timjones.io/626ea18db0663c9ef7d1257940b7a195

PS: I'm actually more than happy to have task-payloads in my shaders - so please keep :)

nhaehnle commented 1 year ago

@Try If you do that, the shader is invalid. And so the consequences are simply the usual consequence for invalid shaders. For example, the Vulkan driver is allowed to crash if you try to create a pipeline that contains the shader. Or perhaps the pipeline compiles successfully, but then when you use it the GPU hangs. Or maybe you get lucky and you just get an error result. The point is, it's undefined behavior and so anything can happen.

Tobski commented 1 year ago

@Try yep what @nhaehnle said. It's an invalid shader. spirv-val is supposed to catch that after compilation and I thought that it would run that automatically as part of glslang, but apparently not 🤔.

Update: It looks like shader playground doesn't automatically run spirv-val when compiling via glslang, and there's no way to add that option at the moment. Might be a feature worth asking for

Try commented 1 year ago

Thanks for answers @Tobski , @nhaehnle !

And so the consequences are simply the usual consequence for invalid shaders.

Need only to pass compilation on my end, rest will be handled by engine. (Basically looking for shader-syntax for fancy draw-indirect)

arcady-lunarg commented 2 months ago

Hi, is there any reason that this hasn't been merged yet? It seems like this unmerged PR is the only documentation of this GLSL extension which glslang implemented 3 years ago.

Tobski commented 2 months ago

Hi, is there any reason that this hasn't been merged yet? It seems like this unmerged PR is the only documentation of this GLSL extension which glslang implemented 3 years ago.

The main issue is that the people involved in this extension are all incredibly busy so any time one of us works on it, the other three people won't get to it for a while. We should probably just merge this as-is at this point and consider any further adjustments as issues. I don't think there's any major outstanding feedback that still needs addressing?

@gnl21 can we hit merge on this?

gnl21 commented 2 months ago

@tobski, there's a thread above where you asking about a patch and I wasn't sure what the status there was. I'm happy for this to be merged if you are, but there was no answer to that question above.

Tobski commented 2 months ago

Let's get it in as-is, if we land the patch we'll rev the extension or add a new extension.