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

Enable exprs for GLSL binding layout qualifiers #5807

Merged
merged 10 commits into from
Dec 10, 2024

Conversation

fairywreath
Copy link
Contributor

@fairywreath fairywreath commented Dec 9, 2024

Closes #5690, partially closes #2174.

Enable const integer literal expressions for GLSL set and binding layout qualifiers. This is only applied for set and binding qualifiers but this PR sets up a good foundation to also modify other GLSL layout qualifiers in the future. Additionally, this PR introduces a change to error out on unrecognized GLSL layout qualifiers instead of silently ignoring.

@fairywreath fairywreath requested a review from a team as a code owner December 9, 2024 08:48
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

Looks good overall! Thank you for contributing the patch!

bool addNode = true;

const auto& name = uncheckedAttr->getKeywordName()->text;
if (name == "binding" || name == "set")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using name comparisons, just dynamic cast uncheckedAttr:

if (auto bindingAttr = as<UncheckedGLSLBindingLayoutAttribute>(uncheckedAttr))
{
     ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for "binding" and "offset" but "set" and other glsl layout modifier names that will be added in the future do(or will) not have explicit types. Binding and offset explicitly have them just to work around the edge case for atomic counters.

source/slang/slang-check-modifier.cpp Show resolved Hide resolved
tests/glsl/layout-qualifier-exprs.slang Outdated Show resolved Hide resolved
@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Dec 9, 2024
csyonghe
csyonghe previously approved these changes Dec 10, 2024
bool addNode = true;

const auto& name = uncheckedAttr->getKeywordName()->text;
if (as<UncheckedGLSLBindingLayoutAttribute>(uncheckedAttr) || name == "set")
Copy link
Collaborator

Choose a reason for hiding this comment

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

May as well add a type for UncheckedGLSLSetLayoutAttribute and get rid of the name comparison here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I will clean this up in another patch that implements the other GLSL layout modifiers to exprs. I originally did not want to add a new Unchecked AST modifier node for every modifier but this is indeed cleaner.

@csyonghe
Copy link
Collaborator

There are still some test regressions that need to be fixed.

@fairywreath
Copy link
Contributor Author

fairywreath commented Dec 10, 2024

layout(rgba6f) // image format not supported by slang
RWTexture2D<float4> texture;

This code does not compile because my change introduces errors on unknown qualifiers, including unsupported/unknown image formats. The current behavior silently ignores this - should I keep this behavior and at least introduce a warning?

EDIT: my latest change still enforces the error, do tell me if this is not preferred.

@csyonghe
Copy link
Collaborator

Looks good to me. Error on unknown qualifier is good.

@csyonghe csyonghe merged commit 523e9f0 into shader-slang:master Dec 10, 2024
8 checks passed
@jkwak-work
Copy link
Collaborator

jkwak-work commented Dec 12, 2024

I am changing the label to be "pr: breaking change", because this change can cause errors on the user applications that were working fine before.

@jkwak-work jkwak-work added pr: breaking change PRs with breaking changes and removed pr: non-breaking PRs without breaking changes labels Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: breaking change PRs with breaking changes
Projects
None yet
3 participants