Enable exprs for GLSL binding layout qualifiers (shader-slang#5807)

* Allow glsl set and binding layout qualifiers to be compile time integer exprs * add new tests * add comments * cleanup on asserts * addressed review comments and cleanup * fix missing set expr in test * fixed tests and cleanup --------- Co-authored-by: Yong He <[email protected]>
jkwak-work · Dec 10, 2024 · 523e9f0 · 523e9f0
1 parent 34497ae
commit 523e9f0
Show file tree

Hide file tree

Showing 13 changed files with 357 additions and 90 deletions.
diff --git a/source/slang/slang-ast-modifier.h b/source/slang/slang-ast-modifier.h
@@ -693,6 +693,38 @@ class UncheckedAttribute : public AttributeBase
     Scope* scope = nullptr;
 };
 
+// A GLSL layout qualifier whose value has not yet been resolved or validated.
+class UncheckedGLSLLayoutAttribute : public AttributeBase
+{
+    SLANG_AST_CLASS(UncheckedGLSLLayoutAttribute)
+
+    SLANG_UNREFLECTED
+};
+
+// GLSL `binding` layout qualifier, does not include `set`.
+class UncheckedGLSLBindingLayoutAttribute : public UncheckedGLSLLayoutAttribute
+{
+    SLANG_AST_CLASS(UncheckedGLSLBindingLayoutAttribute)
+
+    SLANG_UNREFLECTED
+};
+
+// GLSL `set` layout qualifier, does not include `binding`.
+class UncheckedGLSLSetLayoutAttribute : public UncheckedGLSLLayoutAttribute
+{
+    SLANG_AST_CLASS(UncheckedGLSLSetLayoutAttribute)
+
+    SLANG_UNREFLECTED
+};
+
+// GLSL `offset` layout qualifier.
+class UncheckedGLSLOffsetLayoutAttribute : public UncheckedGLSLLayoutAttribute
+{
+    SLANG_AST_CLASS(UncheckedGLSLOffsetLayoutAttribute)
+
+    SLANG_UNREFLECTED
+};
+
 // A `[name(arg0, ...)]` style attribute that has been validated.
 class Attribute : public AttributeBase
 {
@@ -854,6 +886,14 @@ class GLSLOffsetLayoutAttribute : public Attribute
     int64_t offset;
 };
 
+// Implicitly added offset qualifier when no offset is specified.
+class GLSLImplicitOffsetLayoutAttribute : public AttributeBase
+{
+    SLANG_AST_CLASS(GLSLImplicitOffsetLayoutAttribute)
+
+    SLANG_UNREFLECTED
+};
+
 class GLSLSimpleIntegerLayoutAttribute : public Attribute
 {
     SLANG_AST_CLASS(GLSLSimpleIntegerLayoutAttribute)

diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
@@ -614,6 +614,17 @@ struct ImplicitCastMethodKey
     }
 };
 
+/// Used to track offsets for atomic counter storage qualifiers.
+struct GLSLBindingOffsetTracker
+{
+public:
+    void setBindingOffset(int binding, int64_t byteOffset);
+    int64_t getNextBindingOffset(int binding);
+
+private:
+    Dictionary<int, int64_t> bindingToByteOffset;
+};
+
 /// Shared state for a semantics-checking session.
 struct SharedSemanticsContext : public RefObject
 {
@@ -645,6 +656,8 @@ struct SharedSemanticsContext : public RefObject
     List<ModuleDecl*> importedModulesList;
     HashSet<ModuleDecl*> importedModulesSet;
 
+    GLSLBindingOffsetTracker m_glslBindingOffsetTracker;
+
 public:
     SharedSemanticsContext(
         Linkage* linkage,
@@ -705,6 +718,8 @@ struct SharedSemanticsContext : public RefObject
         InheritanceCircularityInfo* next = nullptr;
     };
 
+    GLSLBindingOffsetTracker* getGLSLBindingOffsetTracker() { return &m_glslBindingOffsetTracker; }
+
     /// Get the processed inheritance information for `type`, including all its facets
     InheritanceInfo getInheritanceInfo(
         Type* type,
@@ -1055,6 +1070,11 @@ struct SemanticsContext
 
     OrderedHashSet<Type*>* getCapturedTypePacks() { return m_capturedTypePacks; }
 
+    GLSLBindingOffsetTracker* getGLSLBindingOffsetTracker()
+    {
+        return m_shared->getGLSLBindingOffsetTracker();
+    }
+
 private:
     SharedSemanticsContext* m_shared = nullptr;
 
@@ -1647,6 +1667,10 @@ struct SemanticsVisitor : public SemanticsContext
         UncheckedAttribute* uncheckedAttr,
         ModifiableSyntaxNode* attrTarget);
 
+    AttributeBase* checkGLSLLayoutAttribute(
+        UncheckedGLSLLayoutAttribute* uncheckedAttr,
+        ModifiableSyntaxNode* attrTarget);
+
     Modifier* checkModifier(
         Modifier* m,
         ModifiableSyntaxNode* syntaxNode,

diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp
@@ -32,6 +32,7 @@ ConstantIntVal* SemanticsVisitor::checkConstantIntVal(Expr* expr)
         IntegerConstantExpressionCoercionType::AnyInteger,
         nullptr,
         ConstantFoldingKind::CompileTime);
+
     if (!intVal)
         return nullptr;
 
@@ -524,17 +525,32 @@ Modifier* SemanticsVisitor::validateAttribute(
         }
 
         // TODO(JS): Prior validation currently doesn't ensure both args are ints (as specified in
-        // core.meta.slang), so check here to make sure they both are
-        auto binding = checkConstantIntVal(attr->args[0]);
-        auto set = checkConstantIntVal(attr->args[1]);
+        // core.meta.slang), so check here to make sure they both are.
+        //
+        // Binding attribute may also be created from GLSL style layout qualifiers where only one of
+        // the args are specified, hence check for each individually.
+        ConstantIntVal* binding = nullptr;
+        if (attr->args[0])
+            binding = checkConstantIntVal(attr->args[0]);
+
+        ConstantIntVal* set = nullptr;
+        if (attr->args[1])
+            set = checkConstantIntVal(attr->args[1]);
 
-        if (binding == nullptr || set == nullptr)
+        if (!binding && !set)
         {
             return nullptr;
         }
 
-        bindingAttr->binding = int32_t(binding->getValue());
-        bindingAttr->set = int32_t(set->getValue());
+        if (binding)
+        {
+            bindingAttr->binding = int32_t(binding->getValue());
+        }
+
+        if (set)
+        {
+            bindingAttr->set = int32_t(set->getValue());
+        }
     }
     else if (auto simpleLayoutAttr = as<GLSLSimpleIntegerLayoutAttribute>(attr))
     {
@@ -1430,6 +1446,97 @@ bool isModifierAllowedOnDecl(bool isGLSLInput, ASTNodeType modifierType, Decl* d
     }
 }
 
+void GLSLBindingOffsetTracker::setBindingOffset(int binding, int64_t byteOffset)
+{
+    bindingToByteOffset.set(binding, byteOffset);
+}
+
+int64_t GLSLBindingOffsetTracker::getNextBindingOffset(int binding)
+{
+    int64_t currentOffset;
+    if (bindingToByteOffset.addIfNotExists(binding, 0))
+        currentOffset = 0;
+    else
+        currentOffset = bindingToByteOffset.getValue(binding) + sizeof(uint32_t);
+
+    bindingToByteOffset.set(binding, currentOffset + sizeof(uint32_t));
+    return currentOffset;
+}
+
+AttributeBase* SemanticsVisitor::checkGLSLLayoutAttribute(
+    UncheckedGLSLLayoutAttribute* uncheckedAttr,
+    ModifiableSyntaxNode* attrTarget)
+{
+    SLANG_ASSERT(uncheckedAttr->args.getCount() == 1);
+
+    Attribute* attr = nullptr;
+
+    // False if the current unchecked attribute node is deleted and does not result in a new checked
+    // attribute.
+    bool addNode = true;
+
+    if (as<UncheckedGLSLBindingLayoutAttribute>(uncheckedAttr) ||
+        as<UncheckedGLSLSetLayoutAttribute>(uncheckedAttr))
+    {
+        // Binding and set are coupled together as a descriptor table slot resource for codegen.
+        // Attempt to retrieve and annotate an existing binding attribute or create a new one.
+        attr = attrTarget->findModifier<GLSLBindingAttribute>();
+        if (!attr)
+        {
+            attr = m_astBuilder->create<GLSLBindingAttribute>();
+        }
+        else
+        {
+            addNode = false;
+        }
+
+        // `validateAttribute`, which will be called to parse the binding arguments, also accepts
+        // modifiers from vk::binding and gl::binding where both set and binding are specified.
+        // Binding is the first and set is the second argument - specify them explicitly here.
+        if (as<UncheckedGLSLBindingLayoutAttribute>(uncheckedAttr))
+        {
+            uncheckedAttr->args.add(nullptr);
+        }
+        else
+        {
+            uncheckedAttr->args.add(uncheckedAttr->args[0]);
+            uncheckedAttr->args[0] = nullptr;
+        }
+
+        SLANG_ASSERT(uncheckedAttr->args.getCount() == 2);
+    }
+    else if (as<UncheckedGLSLOffsetLayoutAttribute>(uncheckedAttr))
+    {
+        attr = m_astBuilder->create<GLSLOffsetLayoutAttribute>();
+    }
+    else
+    {
+        getSink()->diagnose(uncheckedAttr, Diagnostics::unrecognizedGLSLLayoutQualifier);
+    }
+
+    if (attr)
+    {
+        attr->keywordName = uncheckedAttr->keywordName;
+        attr->originalIdentifierToken = uncheckedAttr->originalIdentifierToken;
+        attr->args = uncheckedAttr->args;
+        attr->loc = uncheckedAttr->loc;
+
+        // Offset constant folding computation is deferred until all other modifiers are checked to
+        // ensure bindinig is checked first.
+        if (!as<GLSLOffsetLayoutAttribute>(attr))
+        {
+            validateAttribute(attr, nullptr, attrTarget);
+        }
+    }
+
+    if (!addNode)
+    {
+        attr = nullptr;
+    }
+
+    return attr;
+}
+
 Modifier* SemanticsVisitor::checkModifier(
     Modifier* m,
     ModifiableSyntaxNode* syntaxNode,
@@ -1455,6 +1562,21 @@ Modifier* SemanticsVisitor::checkModifier(
         return checkedAttr;
     }
 
+    if (auto glslLayoutAttribute = as<UncheckedGLSLLayoutAttribute>(m))
+    {
+        return checkGLSLLayoutAttribute(glslLayoutAttribute, syntaxNode);
+    }
+
+    if (const auto glslImplicitOffsetAttribute = as<GLSLImplicitOffsetLayoutAttribute>(m))
+    {
+        auto offsetAttr = m_astBuilder->create<GLSLOffsetLayoutAttribute>();
+        offsetAttr->loc = glslImplicitOffsetAttribute->loc;
+
+        // Offset constant folding computation is deferred until all other modifiers are checked to
+        // ensure bindinig is checked first.
+        return offsetAttr;
+    }
+
     if (auto decl = as<Decl>(syntaxNode))
     {
         auto moduleDecl = getModuleDecl(decl);
@@ -1903,6 +2025,31 @@ void SemanticsVisitor::checkModifiers(ModifiableSyntaxNode* syntaxNode)
     // install the new list of modifiers on the declaration
     syntaxNode->modifiers.first = resultModifiers;
 
+    // GLSL offset layout qualifiers are resolved after all other modifiers are checked to ensure
+    // binding layout qualifiers are processed first.
+    if (auto glslOffsetAttribute = syntaxNode->findModifier<GLSLOffsetLayoutAttribute>())
+    {
+        if (const auto glslBindingAttribute = syntaxNode->findModifier<GLSLBindingAttribute>())
+        {
+            if (glslOffsetAttribute->args.getCount() == 0)
+            {
+                glslOffsetAttribute->offset = getGLSLBindingOffsetTracker()->getNextBindingOffset(
+                    glslBindingAttribute->binding);
+            }
+            else if (const auto constVal = checkConstantIntVal(glslOffsetAttribute->args[0]))
+            {
+                glslOffsetAttribute->offset = uint64_t(constVal->getValue());
+                getGLSLBindingOffsetTracker()->setBindingOffset(
+                    glslBindingAttribute->binding,
+                    glslOffsetAttribute->offset);
+            }
+        }
+        else
+        {
+            getSink()->diagnose(glslOffsetAttribute, Diagnostics::missingLayoutBindingModifier);
+        }
+    }
+
     postProcessingOnModifiers(syntaxNode->modifiers);
 }
 

diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
@@ -1207,6 +1207,13 @@ DIAGNOSTIC(
     Error,
     cannotUseUnsizedTypeInConstantBuffer,
     "cannot use unsized type '$0' in a constant buffer.")
+DIAGNOSTIC(31216, Error, unrecognizedGLSLLayoutQualifier, "GLSL layout qualifier is unrecognized")
+DIAGNOSTIC(
+    31217,
+    Error,
+    unrecognizedGLSLLayoutQualifierOrRequiresAssignment,
+    "GLSL layout qualifier is unrecognized or requires assignment")
+
 
 // Enums