Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support different SPIRV versions. #4254

Merged
merged 7 commits into from
Jun 2, 2024
Merged

Conversation

csyonghe
Copy link
Collaborator

@csyonghe csyonghe commented Jun 1, 2024

This change extends our SPIRV backend to generate SPIRV 1.3 through 1.6.

SPIRV versions can be explicitly specified via the -profile option.

Added diagnostic message when trying to use direct spirv backend to generate spirv 1.0, 1.1 and 1.2, and directs the user to use -emit-spirv-via-glsl for older SPIR-Vs.

The main change here is :

  1. make sure we declare necessary extensions when producing older SPIRVs.
  2. emit code for UAVs differently when generating spirv 1.3. We will declare UAV buffer as BufferBlock in Uniform storage class when producing SPIRV 1.3, and declare them as Block in StorageBuffer storage class when producing 1.4 and later.
  3. emit code for discard differently when generating spirv 1.6. We will use OpDemoteToHelperInvocation for spirv 1.6 and OpKill for 1.5 and earlier.

Closes #4142.
Closes #3297.

@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Jun 1, 2024
@csyonghe csyonghe changed the title Spirv Support different SPIRV versions. Jun 1, 2024
jkwak-work
jkwak-work previously approved these changes Jun 1, 2024
Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me.

I noticed that discard was removed from "TerminatorInst".
I am not sure if it will have any unexpected impact on lines like,

if (as<IRTerminatorInst>(inst))

There are bunch of them.

{
CapabilitySet latestSpirvCapSet = CapabilitySet(CapabilityName::spirv_latest);
auto latestSpirvCapSetElements = latestSpirvCapSet.getAtomSets()->getElements<CapabilityAtom>();
result = (CapabilityName)latestSpirvCapSetElements[latestSpirvCapSetElements.getCount() - 2]; //-1 gets shader stage
Copy link
Collaborator

@jkwak-work jkwak-work Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a question.
What does "-2" do here?
Is "-1" for stage and "-2" for target?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was done by Ariel. And you are right, -1 is stage because all stage atoms are defined after target atoms.

Comment on lines 3161 to 3177
switch (spirvVersion.m_major)
{
case 1:
{
switch (spirvVersion.m_minor)
{
case 0: atom = CapabilityName::spirv_1_0; break;
case 1: atom = CapabilityName::spirv_1_1; break;
case 2: atom = CapabilityName::spirv_1_2; break;
case 3: atom = CapabilityName::spirv_1_3; break;
case 4: atom = CapabilityName::spirv_1_4; break;
case 5: atom = CapabilityName::spirv_1_5; break;
case 6: atom = CapabilityName::spirv_1_6; break;
}
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add default and have assertion failure to help the case where we add more version numbers later.

@@ -450,10 +450,10 @@ bool TEST_texture_float()
// METALLIB: call {{.*}}.sample_texture_2d_array.v4f32(
&& all(Tv(1) == t2DArray.SampleLevel(samplerState, float3(u, u, 0), 0, int2(1, 1)))

#ifndef EXCLUDE_DEPTH_TEXTURE // Our vulkan backend don't support SampleCmp from a rgb texture.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metal doesn't support RGB for the depth texture either.
These tests under "SampleCmp()" use a scalar float type textures; not rgb
I am not sure why you had to disable those tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because our test infra is creating an input texture of rgba format without the depth_sample capability flag and bind it to the pipeline. This leads to a Vulkan validation error in debug build. Until we enhance our test infra to be able to specify depth textures, we can't run the test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I guess it would fail for Metal too if it actually ran.

@csyonghe
Copy link
Collaborator Author

csyonghe commented Jun 1, 2024

This is intended. We no longer treat it as a terminator inst and instead just treat it as a normal operation. This will preserve the control flow after the discard operation so we generate correct code for DemoteToHelperInvocations.

@csyonghe csyonghe merged commit c5a453e into shader-slang:master Jun 2, 2024
17 checks passed
@csyonghe csyonghe mentioned this pull request Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
2 participants