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

Allow Offset in addition to ConstOffset for texture operations #3765

Open
rg3igalia opened this issue Oct 18, 2024 · 2 comments · May be fixed by #3782
Open

Allow Offset in addition to ConstOffset for texture operations #3765

rg3igalia opened this issue Oct 18, 2024 · 2 comments · May be fixed by #3782

Comments

@rg3igalia
Copy link
Contributor

As part of an upcoming Vulkan maintenance extension I'm developing CTS tests for, we want to allow using Offset in addition to ConstOffset for image operations. This was already possible for image gather operations, but not for others, and is mainly enforced by VUID-StandaloneSpirv-Offset-04663. There are other VUIDs affected by this but that's the main one. On the SPIR-V side there's no such restriction, so SPIR-V doesn't need any special capability nor a new extension to support this behavior.

While developing CTS tests I managed to get what I wanted from glslang by applying the following diff.

diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp
index 215ccd99..b187ce8a 100644
--- a/glslang/MachineIndependent/ParseHelper.cpp
+++ b/glslang/MachineIndependent/ParseHelper.cpp
@@ -2332,9 +2332,7 @@ void TParseContext::builtInOpCheck(const TSourceLoc& loc, const TFunction& fnCan
                                     arg0->getType().getSampler().shadow;
             if (f16ShadowCompare)
                 ++arg;
-            if (! (*argp)[arg]->getAsTyped()->getQualifier().isConstant())
-                error(loc, "argument must be compile-time constant", "texel offset", "");
-            else if ((*argp)[arg]->getAsConstantUnion()) {
+            else if (((*argp)[arg]->getAsTyped()->getQualifier().isConstant()) && (*argp)[arg]->getAsConstantUnion()) {
                 const TType& type = (*argp)[arg]->getAsTyped()->getType();
                 for (int c = 0; c < type.getVectorSize(); ++c) {
                     int offset = (*argp)[arg]->getAsConstantUnion()->getConstArray()[c].getIConst();

But I'm not used to glslang's code so I don't know if my changes are correct and complete, and I'm fairly positive I'm breaking several tests with that change. Could someone take a look at this? Ideally I'd like glslang to support this so I can just update the glslang commit reference in CTS and know this is going to work.

/cc @spencer-lunarg since this is related to validation too.

@arcady-lunarg
Copy link
Contributor

The restriction on the offset having to be a constant is actually a restriction from the GLSL spec (which was probably inherited by SPIR-V and Vulkan), so I think it might be best to make a GLSL extension spec for this and have this enabled by that extension.

@rg3igalia
Copy link
Contributor Author

We have the extension now: KhronosGroup/GLSL#264

I'll try to create an MR with an implementation this week.

@rg3igalia rg3igalia linked a pull request Oct 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants