Invalid SPIR-V (type mismatch) when passing acceleration structure constructor as parameter

[andfau-arm]

Reproducing shader:

#version 460 
#extension GL_EXT_ray_query : enable
#extension GL_EXT_ray_tracing : enable

layout(location = 0) in flat uvec2 AS;
            
void doQuery(accelerationStructureEXT as)
{   
    rayQueryEXT query;
    rayQueryInitializeEXT(query, as, gl_RayFlagsTerminateOnFirstHitEXT, 0xFF, vec3(0), 0.01, vec3(1, 0, 0), 42.0); 
}

void main(void)
{
    doQuery(accelerationStructureEXT(AS));
}

The generated SPIR-V for the doQuery function has this signature:

       %void = OpTypeVoid
          %6 = OpTypeAccelerationStructureKHR
%_ptr_UniformConstant_6 = OpTypePointer UniformConstant %6
          %8 = OpTypeFunction %void %_ptr_UniformConstant_6

Note the parameter's type is a UniformConstant pointer to OpTypeAccelerationStructureKHR.

However, the generated SPIR-V for the function call looks like this:

         %31 = OpConvertUToAccelerationStructureKHR %6 %30
         %32 = OpFunctionCall %void %doQuery_as1_ %31

Note that the argument's type is simply OpTypeAccelerationStructureKHR.

This is a type mismatch and spirv-val rejects this as invalid SPIR-V.

Presumably glslang either should:

• Reject the shader with some error message
• Accept the shader and produce SPIR-V with a function signature that allows this to work, e.g. having the parameter type be OpTypeAccelerationStructureKHR directly, or a pointer in Function/Private storage class

[greg-lunarg]

@dgkoch Can you please take this or find someone who can?

[greg-lunarg]

@dgkoch Ping :)

[dgkoch]

I've seen and am trying to get someone to look at it.

[andfau-arm]

I reported this as a glslang bug, but another way to look at this is as a defect in the GLSL extension instead.

[alelenv]

I will look into this

[dgkoch]

Discussed in the ray tracing TSG 10/14 - general feeling was that passing accerationStructures to functions should work like passing textures and other bindings.

If that doesn't work out for some reason, let us know and we can revisit.

[andfau-arm]

Discussed in the ray tracing TSG 10/14 - general feeling was that passing accerationStructures to functions should work like passing textures and other bindings.

If that doesn't work out for some reason, let us know and we can revisit.

A similar thing for samplers is not accepted by glslang:

#version 460

layout(set=0, binding=0) uniform sampler s;
layout(set=0, binding=1) uniform texture2D t;

vec4 doSample(sampler2D s2d)
{
    return texture(s2d, vec2(0.0));
}

void main()
{
    doSample(sampler2D(t, s));
}

ERROR: sampler.frag:13: 'call argument' : sampler constructor must appear at point of use 
ERROR: sampler.frag:13: '' : compilation terminated 
ERROR: 2 compilation errors.  No code generated.


SPIR-V is not generated for failed compile or link

This is another case where glslang uses UniformConstant pointers for these parameters, so it would have the same problem as for acceleration structures if it were to allow this.

It is not clear to me whether this is actually disallowed by the GLSL spec. These parts suggest the spirit of the spec is to disallow this:

The result of a texture-combined sampler constructor cannot be assigned to a variable

there is no control flow construct (e.g., ?:) that consumes any sampler type

Yet the spec also says:

Texture-combined sampler constructors can only be consumed by a function parameter.

So far as I know, the Vulkan and SPIR-V specifications do not constrain glslang here. It could support this.

[greg-lunarg]

As far as the above sampler2D constructor example goes, the current SPIR-V spec says this:

– All OpSampledImage instructions must be in the same block in which their Result are consumed.

From the GL_KHR_vulkan_glsl extension text:

Constructors can then be used to combine a sampler and a texture **at the
point of making a texture lookup call**

So I am pretty sure glslang is correct in giving an error here.

[alelenv]

Discussed in the ray tracing TSG 10/14 - general feeling was that passing accerationStructures to functions should work like passing textures and other bindings.

If that doesn't work out for some reason, let us know and we can revisit.

I dont think this is same and will not work. Opaque types will always have UniformConstant as storage class (We don't support ARB/NV_bindless_texture yet which will have same problem as mentioned below)

We can pass acceleration structure declared as a uniform or create it on stack using the 64bit integer to acceleration structure constructor and pass it to a function, which causes the address space mismatch issue

We had similar issue with rayQuery objects where we promoted all of them to Private storage class to avoid this issue
I think at TSG we were wondering of adding an 'addrspacecast' SPIRV opcode to solve this, but unsure if thats the right way to go

Note I won't be able to look into this till mid January at earliest

[greg-lunarg]

In general, true opaque types like textures and samplers cannot be stored in local variables and cannot be constructed or converted from non-opaque types. They are passed to functions by reference ie. pointers to the objects are passed.

The question then is: what is accelerationStructureEXT? Is it a true opaque type?

GL_EXT_ray_query says that accelerationStructureEXT is an opaque type and an object of that type is a handle. It also says that a member of a structure cannot be of that type. This suggests that the size of such an object is not defined.

All of this makes the ability to convert from uvec2 very curious.

We can ... create it (acceleration structure) on stack using the 64bit integer to acceleration structure constructor and pass it to a function, which causes the address space mismatch issue

Where is this ability defined?

[alelenv]

Where is this ability defined?

In https://github.com/KhronosGroup/GLSL/blob/master/extensions/ext/GLSL_EXT_ray_tracing.txt
148 : accelerationStructureEXT constructor -> OpConvertUToAccelerationStructureKHR

There is a example showing its usage further down

[greg-lunarg]

FWIW, here is the definition of OpConvertUToAccelerationStructureKHR:

https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/ray_common/opconvertutoaccelerationstructure_table.txt

[greg-lunarg]

I was asked yesterday at the SPIR-V WG to look into this from the glslang developer perspective.

Right now, on the function callee side, glslang is doing what it normally does for all parameters of opaque type: it is creating an OpFunctionParameter of type pointer to the opaque type with a Uniform storage class, IE it is setting up to receive an l-value IE call by reference.

Unfortunately, on the caller side, given the function declared as such, it is not clear what it should or can do. The function needs a pointer / l-value, but OpConvertUToAccelerationStructureEXT returns an r-value.

Here is a possible solution that would not require any changes to the RT spec:

I cannot find a rule in SPIR-V or Vulkan that says that an OpFunctionParameter cannot be an opaque type. So one possible solution is to always pass accelerationStructureEXT to functions by value. On the callee side, declare the OpFunctionParameter as type accelerationStructureEXT (rather than pointer to accelerationStructureEXT). On the caller side, if it is already an r-value, pass it, other de-reference it first.

We probably want to check with the IHVs that they are fine with passing these into function calls by value rather than reference, but right now, I cannot find a rule against it.

Otherwise we might have to create a version of OpConvertUToAccelerationStructureEXT which takes an l-value and returns an l-value, but the fact that a pointer contains a storage class will complicate this approach.

[dgkoch]

Otherwise we might have to create a version of OpConvertUToAccelerationStructureEXT which takes an l-value and returns an l-value, but the fact that a pointer contains a storage class will make this difficult.

The simplest WAR would just be for the shader author to pass the AS around as uvec2/uint64_t value and not to do the accelerationStructureEXT() cast/conversion until right before calling rayQueryInitializeEXT (or whatever other function that takes an AS).

#version 460 
#extension GL_EXT_ray_query : enable
#extension GL_EXT_ray_tracing : enable

layout(location = 0) in flat uvec2 AS;
            
void doQuery(uvec2 as)
{   
    rayQueryEXT query;
    rayQueryInitializeEXT(query, accelerationStructureEXT(as), gl_RayFlagsTerminateOnFirstHitEXT, 0xFF, vec3(0), 0.01, vec3(1, 0, 0), 42.0); 
}

void main(void)
{
    doQuery(AS);
}

We could add a recommendation/clarification to this effect in the GLSL extensions if needed.
I guess separately maybe there should be an investigation to see what would be needed to make this more user friendly..

[greg-lunarg]

At the SPIR-V WG today, it was decided to implement this by passing the acceleration structure by value.

@dgkoch Do you have someone who can do this work?

[dgkoch]

Likely not for a while - are you able to take a stab at it?

[greg-lunarg]

Not immediately. I also have a few concerns about this approach after thinking about it for a bit more. Once they are sorted out and they hold up, I will post them here.

[greg-lunarg]

I have resolved all my concerns and believe we can go ahead with the current plan.

[andfau-arm]

What happens with a parameter that takes an array of acceleration structures?

[greg-lunarg]

What happens with a parameter that takes an array of acceleration structures?

I think it would work to continue to pass an array of AS by reference ie l-value. Right?

[andfau-arm]

Sure, but then you can't create an array with a constructor or local variable. Whether that's worth supporting, I don't know.

[greg-lunarg]

Sure, but then you can't create an array with a constructor or local variable. Whether that's worth supporting, I don't know.

I think it is ok if these things cannot be created. If someone has a counter-example, please share.