Fix IR emit logic for methods in struct types (shader-slang#791)

There was a bug in the logic for emitting initial IR, such that it was neglecting to emit "methods" (member functions) unless they were also referenced by a non-member (global) function, or were needed to satisfy an interface requirement. This would only matter for `import`ed modules, since for non-`import`ed code, anything relevant would be referenced by the entry point so that the problem would never surface.

This change fixes the underlying problem by adding a step to the IR lowering pass called `ensureAllDeclsRec` that makes sure that not only global-scope declarations, but also anything nested under a `struct` type gets emitted to the initial IR module.

There are also a few unrelated fixes in this PR, which are things I ran into while making the fix:

* Deleted support for the (long gone) `IRDeclRef` type in our `slang.natvis` file

* Added support for visualizing the value of IR string and integer literals when they appear in the debugger

* Fixed IR dumping logic to not skip emitting `struct` and `interface` instructions. Switching those to inherit from `IRType` accidentally affected how they get printed in IR dumps by default.

* Fixed up the IR linking logic so that it correctly takes `[export]` decorations into account, so that an exported definition will always be taken over any other (unless the latter is more specialized for the target). I initially implemented this in an attempt to fix the original issue, but found it wasn't a fix for the root cause. It is still a better approach than what was implemented previously, so I'm leaving it in place.

Author: Tim Foley

source/slang/ir-link.cpp

@@ -855,7 +855,15 @@ bool isBetterForTarget(
     if(newLevel != oldLevel)
         return UInt(newLevel) > UInt(oldLevel);
 
-    // All other factors being equal, a definition is
+    // All preceding factors being equal, an `[export]` is better
+    // than an `[import]`.
+    //
+    bool newIsExport = newVal->findDecoration<IRExportDecoration>() != nullptr;
+    bool oldIsExport = oldVal->findDecoration<IRExportDecoration>() != nullptr;
+    if(newIsExport != oldIsExport)
+        return newIsExport;
+
+    // All preceding factors being equal, a definition is
     // better than a declaration.
     auto newIsDef = isDefinition(newVal);
     auto oldIsDef = isDefinition(oldVal);

source/slang/ir.cpp

@@ -3118,6 +3118,22 @@ namespace Slang
         if(as<IRConstant>(inst))
             return true;
 
+        // We are going to have a general rule that
+        // a type should be folded into its use site,
+        // which improves output in most cases, but
+        // we would like to not apply that rule to
+        // "nominal" types like `struct`s.
+        //
+        switch( inst->op )
+        {
+        case kIROp_StructType:
+        case kIROp_InterfaceType:
+            return false;
+
+        default:
+            break;
+        }
+
         if(as<IRType>(inst))
             return true;
 

source/slang/lower-to-ir.cpp

@@ -4922,7 +4922,6 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
 
         IRStructType* irStruct = subBuilder->createStructType();
         addNameHint(context, irStruct, decl);
-
         addLinkageDecoration(context, irStruct, decl);
 
         subBuilder->setInsertInto(irStruct);
@@ -4959,15 +4958,16 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
                 fieldType);
         }
 
-        // TODO: we should enumerate the non-field members of the type
-        // as well, and ensure those have been emitted (e.g., any
-        // member functions).
+        // There may be members not handled by the above logic (e.g.,
+        // member functions), but we will not immediately force them
+        // to be emitted here, so as not to risk a circular dependency.
+        //
+        // Instead we will force emission of all children of aggregate
+        // type declarations later, from the top-level emit logic.
 
         irStruct->moveToEnd();
-
         addTargetIntrinsicDecorations(irStruct, decl);
 
-
         return LoweredValInfo::simple(finishOuterGenerics(subBuilder, irStruct));
     }
 
@@ -6147,6 +6147,31 @@ static void lowerEntryPointToIR(
     }
 }
 
+    /// Ensure that `decl` and all relevant declarations under it get emitted.
+static void ensureAllDeclsRec(
+    IRGenContext*   context,
+    Decl*           decl)
+{
+    ensureDecl(context, decl);
+
+    // Note: We are checking here for aggregate type declarations, and
+    // not for `ContainerDecl`s in general. This is because many kinds
+    // of container declarations will already take responsibility for emitting
+    // their children directly (e.g., a function declaration is responsible
+    // for emitting its own parameters).
+    //
+    // Aggregate types are the main case where we can emit an outer declaration
+    // and not the stuff nested inside of it.
+    //
+    if(auto containerDecl = dynamic_cast<AggTypeDecl*>(decl))
+    {
+        for (auto memberDecl : containerDecl->Members)
+        {
+            ensureAllDeclsRec(context, memberDecl);
+        }
+    }
+}
+
 IRModule* generateIRForTranslationUnit(
     TranslationUnitRequest* translationUnit)
 {
@@ -6192,7 +6217,7 @@ IRModule* generateIRForTranslationUnit(
     // been emitted.
     for (auto decl : translationUnit->SyntaxNode->Members)
     {
-        ensureDecl(context, decl);
+        ensureAllDeclsRec(context, decl);
     }
 
 #if 0

source/slang/slang.natvis

@@ -82,6 +82,8 @@
     <Expand>
       <Item Name="[op]">op</Item>
       <Item Name="[type]">typeUse.usedValue</Item>
+      <Item Name="[value]" Condition="op == Slang::kIROp_StringLit">((IRStringLit*)this)->value.stringVal.chars,[((IRStringLit*)this)->value.stringVal.numChars]s8</Item>
+      <Item Name="[value]" Condition="op == Slang::kIROp_IntLit">((IRIntLit*)this)->value.intVal</Item>
       <Synthetic Name="[operands]">
         <DisplayString>{{count = {operandCount}}}</DisplayString>
         <Expand>
@@ -113,15 +115,6 @@
       </Synthetic>
   </Expand>
   </Type>
-  <Type Name="Slang::IRDeclRef">
-    <DisplayString>{{IRDeclRef {declRef}}}</DisplayString>
-    <Expand>
-      <Item Name="[op]">op</Item>
-      <Item Name="[type]">type</Item>
-      <Item Name="[declRef]">declRef</Item>
-      <Item Name="[parent]">parent</Item>
-    </Expand>
-  </Type>
   <Type Name="Slang::IRUse">
     <DisplayString>{{IRUse {usedValue}}}</DisplayString>
     <Expand>

tests/bugs/gl-33-ext.slang

@@ -0,0 +1,8 @@
+// gl-33-ext.slang
+//TEST_IGNORE_FILE:
+
+struct A
+{
+    int state;
+    [mutating] int next() { return state; }
+};

tests/bugs/gl-33.slang

@@ -0,0 +1,28 @@
+// gl-33.slang
+//TEST(compute):COMPARE_COMPUTE:
+
+// Test for GitLab issue # 33
+
+import gl_33_ext;
+
+typedef A B;
+
+int test(int val)
+{
+    B b = { val };
+    return b.next();
+}
+
+//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):dxbinding(0),glbinding(0),out
+RWStructuredBuffer<int> gBuffer;
+
+[numthreads(4, 1, 1)]
+void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
+{
+    uint tid = dispatchThreadID.x;
+
+    int val = tid;
+    val = test(val);
+
+    gBuffer[tid] = val;
+}

tests/bugs/gl-33.slang.expected.txt

@@ -0,0 +1,4 @@
+0
+1
+2
+3