Static const members in nested structs don't compile to SPIR-V

[ChristianReinbold]

Description Accessing static const members in nested structs results in "found unregistered decl". This currently prevents us from implementing some compile-time performance optimizations via metaprogramming, since this issue persists if the nested struct is a template.

Steps to Reproduce Reproducer can be found here: https://godbolt.org/z/brnGYKddj

struct A {
    static const uint value = 6u;
    struct B { static const uint value = 6u; };
};

[[vk::binding(0,4)]] RWStructuredBuffer<uint> a;

// Define some dummy entry point
[shader("raygeneration")]
void main() {
    a[0] = A::value;
    a[0] = A::B::value;
}

cannot be compiled with dxc -T lib_6_4 -spirv -fspv-target-env=vulkan1.2 -enable-16bit-types -HV 2021

Removing the line a[0] = A::B::value; results in successful compilation.

Actual Behavior

<source>:3:34: fatal error: found unregistered decl
    struct B { static const uint value = 6u; };
                                 ^
note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

Compiler returned: 5

Environment

• DXC version: libdxcompiler.so: 1.7(dev;1-481090a8)
• Host Operating System: Compiler Explorer

[ChristianReinbold]

After digging a bit it seems that variable declarations in nested structs are ignored since SpirvEmitter::doRecordDecl does not recurse into record kind sub-declarations. The same observation holds true if the nested struct is templated.

[ChristianReinbold]

After digging a bit more, I ended up initializing static const members not in SpirvEmitter::doRecordDecl, but in DeclResultIdMapper::tryToCreateImplicitConstVar instead. This has the benefit that static const members work out of the box with templates as well. Check out this small patch based on commit 64030a4e01e27c608b6c5f1ab5add43575e086f9.

Note that I also experimented if the constexpr keyword could be enabled to support constexpr functions in static member initialization. I have no idea about all the implications these changes may have (does HLSL even permit the constexpr keyword?), but at least now I seem to be able to do some compile-time metaprogramming with dxc. The following hlsl-file compiles to the correct spir-v.

For now, I will continue tinkering with this patched version of DXC, and maybe come up with a feature or pull request later on if it turns out that we would profit from metaprogramming.

[sudonatalie]

@cassiebeckley Can you take a look at this issue?

RE: constexpr, it is not currently supported by HLSL so requests about it should be directed to the hlsl-specs repository.

[ChristianReinbold]

Hi there, is there any update related to this issue? At the moment I rely on the patch (without the constexpr changes) to build my projects that make use of nested static const variable. If it helps, I can also open up a PR and see if I am able to provide some tests.

[sudonatalie]

@ChristianReinbold I'm not sure what happened with the patch you linked above but it appears to be a binary file, not a regular git patch. If you have a fix though and can add some tests we'd be happy to review a PR.

[ChristianReinbold]

Mhm, for me the patch is a text file. Maybe I messed up the encoding? Notepad++ shows up an utf-16 encoding. Thanks for the update. I will come up with a PR at some point then.

[FoggyMist]

@sudonatalie, here is content of the diff file:

diff --git a/tools/clang/include/clang/Basic/TokenKinds.def b/tools/clang/include/clang/Basic/TokenKinds.def
index 2267b12b7..7326f2d92 100644
--- a/tools/clang/include/clang/Basic/TokenKinds.def
+++ b/tools/clang/include/clang/Basic/TokenKinds.def
@@ -345,7 +345,7 @@ CXX11_KEYWORD(alignas               , 0)
 CXX11_KEYWORD(alignof               , 0)
 CXX11_KEYWORD(char16_t              , KEYNOMS18)
 CXX11_KEYWORD(char32_t              , KEYNOMS18)
-CXX11_KEYWORD(constexpr             , 0)
+CXX11_KEYWORD(constexpr             , KEYHLSL)
 CXX11_KEYWORD(decltype              , 0)
 CXX11_KEYWORD(noexcept              , 0)
 CXX11_KEYWORD(nullptr               , 0)
diff --git a/tools/clang/lib/Parse/ParseDecl.cpp b/tools/clang/lib/Parse/ParseDecl.cpp
index 62034b667..e031c84ca 100644
--- a/tools/clang/lib/Parse/ParseDecl.cpp
+++ b/tools/clang/lib/Parse/ParseDecl.cpp
@@ -3976,7 +3976,9 @@ HLSLReservedKeyword:

     // constexpr
     case tok::kw_constexpr:
-      if (getLangOpts().HLSL) { goto HLSLReservedKeyword; } // HLSL Change - reserved for HLSL
+      // We want the constexpr keyword available for using constexpr functions to initialize 
+      // static const members.
+      // if (getLangOpts().HLSL) { goto HLSLReservedKeyword; } // HLSL Change - reserved for HLSL
       isInvalid = DS.SetConstexprSpec(Loc, PrevSpec, DiagID);
       break;

diff --git a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp
index 9fe150634..f4ec3a974 100644
--- a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp
+++ b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp
@@ -4147,7 +4147,11 @@ DeclResultIdMapper::createRayTracingNVStageVar(spv::StorageClass sc,

 void DeclResultIdMapper::tryToCreateImplicitConstVar(const ValueDecl *decl) {
   const VarDecl *varDecl = dyn_cast<VarDecl>(decl);
-  if (!varDecl || !varDecl->isImplicit())
+  // Make sure to initialize static const members here. Since evaluateValue()
+  // also seems to handle constexpr members in templated and nested types, this
+  // change seems to be enough to support some basic metaprogramming idioms.
+  bool isStatic = varDecl->isStaticDataMember() && varDecl->hasInit();
+  if (!varDecl || !(varDecl->isImplicit() || isStatic))
     return;

   APValue *val = varDecl->evaluateValue();
diff --git a/tools/clang/lib/SPIRV/SpirvEmitter.cpp b/tools/clang/lib/SPIRV/SpirvEmitter.cpp
index 8120ba3b5..39bc528e0 100644
--- a/tools/clang/lib/SPIRV/SpirvEmitter.cpp
+++ b/tools/clang/lib/SPIRV/SpirvEmitter.cpp
@@ -994,6 +994,8 @@ void SpirvEmitter::doDecl(const Decl *decl) {
     doClassTemplateDecl(classTemplateDecl);
   } else if (isa<FunctionTemplateDecl>(decl)) {
     // nothing to do.
+  } else if (isa<TypeAliasDecl>(decl)) {
+    // nothing to do.
   } else {
     emitError("decl type %0 unimplemented", decl->getLocation())
         << decl->getDeclKindName();
@@ -1704,10 +1706,7 @@ void SpirvEmitter::doRecordDecl(const RecordDecl *recordDecl) {
   // RecordDecl. For those defined in the translation unit,
   // their VarDecls do not have initializer.
   for (auto *subDecl : recordDecl->decls()) {
-    if (auto *varDecl = dyn_cast<VarDecl>(subDecl)) {
-      if (varDecl->isStaticDataMember() && varDecl->hasInit())
-        doVarDecl(varDecl);
-    } else if (auto *enumDecl = dyn_cast<EnumDecl>(subDecl)) {
+    if (auto *enumDecl = dyn_cast<EnumDecl>(subDecl)) {
       doEnumDecl(enumDecl);
     }
   }