Added first draft of GLSL_EXT_spirv_intrinsics

[Tobski]

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]

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

[nhaehnle]

There is precedent for constant type parameters that are literals (more frequent) and those that are IDs (less frequent; notably: OpTypeArray, OpTypeCooperativeMatrixNV).

The glslang implementation has a half-baked attempt to support that, but tries to do so by guessing based on whether the parameter is syntactically a literal, which doesn't make much sense.

It seems useful to support ID parameters, though. In nhaehnle/glslang@99ad9a7 I implement this by adding an optional spirv_id qualifier. So:

spirv_type (id = 1234, 2)

results in

%nn = OpType1234 2

while

spirv_type (id = 1235, spirv_id 2) %

results in

%nn = OpType1235 %int_2

(specialization constants are supported).

An alternative path for support would be to invert the default and re-use the existing spirv_literal. That's easy to implement, too, I just took the path that least disturbs existing users of the already-merged implementation of this unmerged extension.

Relatedly, many type instructions have other types as parameters. Is it intentional not to support those? The glslang implementation actually does support it, though KhronosGroup/glslang#2777 would remove it again.

[Tobski]

I'd be unopposed to adding a "spirv_id" qualifier. Bit annoying that it's reversed in hindsight but oh well, live and learn...

Finding a way to fit types in here would also be good, I just couldn't find a neat way in the grammar to make types work, though it seems pre KhronosGroup/glslang#2777 the grammar was accepting this somehow? May want to do that as a follow up extension though.

[Tobski]

@nhaehnle I've added the suggested spirv_id qualifier, is your patch still applicable and could you submit it?

[gnl21]

Is the PR going to be updated in response to this or can we resolve this thread now?

[dgkoch]

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

[Tobski]

@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]

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

[dgkoch]

semi-annual poke..

[amdrexu]

A change is to fix a problem here (KhronosGroup/glslang#3170). We should allow variables in the spirv_decorate_id-parameter-list. This is because OpDecorateId allows such cases.

Suggested change

	constant-expression
	spirv_decorate_id_parameter_list
	spirv_decorate_id_parameter
	spirv_decorate_id_parameter spirv_decorate_id_parameter_list
	spirv_decorate_id_parameter
	variable-identifier
	floating-constant
	integer-constant
	bool-constant

[gnl21]

I think that constant-expression already includes variable identifiers, doesn't it?

[Tobski]

It does, and actually now I look at it it doesn't seem like anything about constant-expression actually requires it to evaluate to a constant value, so maybe this is fine as-is?

[Tobski]

Assuming that constant-expression in the grammar doesn't actually need constants, I think we're good. If not I might need some guidance on what grammar identifier should be in its place...

[gnl21]

constant-expression is an alias in the grammar for conditional-expression that gets used when things have to be constant. It's used for layout qualifiers at the moment, so if you don't intend this to follow similar rules then it should probably say conditional-expression instead. Based on @amdrexu's suggestion it sounds like primary-expression might be the most appropriate though?

[r-potter]

@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]

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]

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]

@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]

@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]

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

[Try]

@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]

@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]

@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]

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]

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]

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]

@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]

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