search

shader-slang / slang

Making it easier to work with shaders
MIT License
1.78k stars 159 forks source link

Support status argument for GatherXXX #4490

Open jkwak-work opened 6 days ago

jkwak-work commented 6 days ago

This commit adds an argument to all texture GatherXXX functions. The new argument is for "status" as described in SM5.0 definision.

Close #4466

ArielG-NV commented 6 days ago

I wonder if we should add a stub for CheckAccessFullyMapped() so code that uses status can compile (status is useless without CheckAccessFullyMapped(), so these functions appear in pairs)?

ArielG-NV commented 6 days ago

I wonder if we should add a stub for CheckAccessFullyMapped() so code that uses status can compile (status is useless without CheckAccessFullyMapped(), so these functions appear in pairs)?

After looking into what status means across platforms, CheckAccessFullyMapped() must be added for tiled/sparse memory accesses to work properly since they signify if the access done was undefined behavior (not all memory is resident with sparse allocations) or not.

HLSL: CheckAccessFullyMapped() GLSL: sparseTexelsResidentARB() with extension GL_ARB_sparse_texture2 SPIRV: OpImageSparseTexelsResident with capability OpCapability SparseResidency

jkwak-work commented 6 days ago

Thanks to Ariel. I didn't realize that the status is for the partial resident texture. That is a very important feature that many of triple A games make use of. We definitely should consider supporting it fully.

jkwak-work commented 5 days ago

I think we need a new issue for fully supporting the partial resident texture feature. This PR is to address the missing status for Gather functions.

ArielG-NV commented 5 days ago

I think we need a new issue for fully supporting the partial resident texture feature. This PR is to address the missing status for Gather functions.

Made an issue for the rest of the operations

jkwak-work commented 5 days ago

If we haven't meaningfully implemented status for Vulkan, we should make it a capability error instead of silently implement it in an incorrect way, that will make it hard for users to identify the problem.

I pushed a change to apply the capability check so that only HLSL will be allowed to use the status variant. However, it seems that there is a bug that VK can still run the function with no problem. I asked @ArielG-NV to take a look.

ArielG-NV commented 4 days ago

I pushed a change to apply the capability check so that only HLSL will be allowed to use the status variant. However, it seems that there is a bug that VK can still run the function with no problem. I asked @ArielG-NV to take a look.

I looked into it, looks like there are 2 issues causing this bug (so far from what I see):

  1. texture_gather has incorrect capabilities (missing hlsl due to joining an any_target with a spirv | glsl | metal)
  2. validateAttribute of RequireCapabilityAttribute never checks if the capability produced is invalid or not. This should be an error.

What happens in this scenario is: texture_gather is missing hlsl, so [require(hlsl, texure_gather)] resolves into Invalid, which is not supposed to be possible, causing problems.

I will make an issue for this.

@jkwak-work do you want the fix to be in a separate PR or apart of your jkwak-work:feature/support_GatherAlpha_with_status_argument branch (I can make a PR to your branch)?

ArielG-NV commented 4 days ago

Seems there is a third issue(?): __target_switch never compares case: capabilities to the parent [require(...)]

This means incorrect capabilities may be labeled by a user for a function

jkwak-work commented 4 days ago

@jkwak-work do you want the fix to be in a separate PR or apart of your jkwak-work:feature/support_GatherAlpha_with_status_argument branch (I can make a PR to your branch)?

I think your PR #4510 will have the fix, right? I will wait for your PR to be merged first and make sure CI goes all green with my PR. Thanks for fixing the issue so promptly.