Move mangled name out of IRGlobalValue (shader-slang#752)

* Move mangled name out of IRGlobalValue

Previously the `IRGlobalValue` type was used as a root for all IR instructions that can have "linkage," in the sense that a definition in one module can satisfy a use in another module.
The mangled symbol name was stored in state directly on each `IRGlobalValue`, which created some complications, and also forced IR instructions that wanted to support linkage to wedge into the hierarchy at that specific point.

This change moves the mangled name out into a decoration: either an `IRImportDecoration` or an `IRExportDecoration`, both of which inherit from `IRLinkageDecoration` which exposes the mangled name.
This change has a few benefits:

* We can now have any kind of instruction be exported/imported, without having to inherit from `IRGlobalValue`. This could potentially let `IRStructType` and `IRWitnessTable` be simplified to just have operand lists instead of dummy chldren as they do today.

* We can now easily have "global values" like functions that explicitly *don't* get linkage, instead of using a null or empty mangled name as a marker.

* We can use the exact opcode on a linkage decoration to distinguish imports from exports, which could be used to more accurately resolve symbols during the linking step.

Other than adding the decorations and making sure that AST->IR lowering adds them, the main changes here are around any code that used `IRGlobalValue`. Variables and parameters of type `IRGlobalValue*` were changed to `IRInst*` easily, so the main challenge was around code that *casts* to `IRGlobalValue*.

In cases where a cast to `IRGlobalValue` also performed a test for the mangled name being non-null/non-empty, we simply switched the code to check for the presence of an `IRLinkageDecoration`, since that is the new way of indicating a value with linakge.

Most of the serious complications arose in `ir.cpp` around the "linking"/target-specialization and generic specialization steps.

The "linking" logic was checking for `IRGlobalValue` to opt into some more complicated cloning logic, and just checking for a linkage decoration here wasn't sufficient since the front-end *does* produce global values without linkage in some cases (e.g., for a function-`static` variable we produce a global variable without linkage). This logic was updated to just check for the cases that used to amount to `IRGlobalValue`s directly by opcode. It might be simpler in the short term to have kept `IRGlobalValue` around to make the existing casts Just Work, but I'm confident that this logic could actually be rewritten for much greater clarity and simplicity and that is the better way forward.

The generic specialization logic was using some really messy code to generate a new mangled name to represent the specialized symbol, and then searching for an existing match for that name.
The original idea there was that an IR module could include "pre-specialized" versions of certain generics to speed up back-end compilation by eliminating the need to specialize in some cases, but this feature has never been implemented so the overhead here is just a waste.
Instead, I moved generic specialization to use a simpler dictionary to map the operands to a `specialize` instruction over to the resulting specialized value.
This allows for some simplifications in the name mangling logic, because it no longer needs to figure out how to produce mangled names from IR instructions representing types/values.

As part of this change I also overhauled the IR emit logic to produce cleaner output by default, borrowing some of the ideas from the logic in `emit.cpp`. IR values are now automatically given names based on their "name hint" decoration, if any, to make the code easier to follow, and I also made it so that types and literals get collapsed into their use sites in a new "simplified" IR dump mode (which is currently the default, with no way to opt into the other mode without tweaking the code). The resulting IR dumps are much nicer to look at, but as a result the one test that involves IR dumping (`ir/string-literal`) doesn't really test what it used to.

One weird issue that came up during testing is that the `transitive-interface` test had previously been producing output that made no sense (that is, the expected output file wasn't really sensible), and somehow these changes were altering its behavior. Changing the test to use `int` values instead of `float` was enough to make the output be what I'd expect, and hand inspection of generating DXBC has me convinced we were compiling the `float` case correctly too. There appears to be some issue around tests with floating-point outputs that we should investigate.

* fixup: C++ declaration order

Author: Tim Foley

source/core/slang-string.cpp

@@ -45,6 +45,23 @@ namespace Slang
 
     // UnownedStringSlice
 
+    bool UnownedStringSlice::startsWith(UnownedStringSlice const& other) const
+    {
+        UInt thisSize = size();
+        UInt otherSize = other.size();
+
+        if (otherSize > thisSize)
+            return false;
+
+        return UnownedStringSlice(begin(), begin() + otherSize) == other;
+    }
+
+    bool UnownedStringSlice::startsWith(char const* str) const
+    {
+        return startsWith(UnownedTerminatedStringSlice(str));
+    }
+
+
     bool UnownedStringSlice::endsWith(UnownedStringSlice const& other) const
     {
         UInt thisSize = size();

source/core/slang-string.h

@@ -133,6 +133,8 @@ namespace Slang
             return !(*this == other);
         }
 
+        bool startsWith(UnownedStringSlice const& other) const;
+        bool startsWith(char const* str) const;
 
         bool endsWith(UnownedStringSlice const& other) const;
         bool endsWith(char const* str) const;

source/slang/emit.cpp

@@ -1,6 +1,7 @@
 // emit.cpp
 #include "emit.h"
 
+#include "../core/slang-writer.h"
 #include "ir-insts.h"
 #include "ir-restructure.h"
 #include "ir-restructure-scoping.h"
@@ -2226,17 +2227,9 @@ struct EmitVisitor
 
 
         // If the instruction has a mangled name, then emit using that.
-        if (auto globalValue = as<IRGlobalValue>(inst))
+        if(auto linkageDecoration = inst->findDecoration<IRLinkageDecoration>())
         {
-            auto mangledName = globalValue->mangledName;
-            if (mangledName)
-            {
-                auto mangledNameText = getText(mangledName);
-                if (mangledNameText.Length() != 0)
-                {
-                    return getText(mangledName);
-                }
-            }
+            return linkageDecoration->getMangledName();
         }
 
         // Otherwise fall back to a construct temporary name
@@ -3411,7 +3404,7 @@ struct EmitVisitor
         // (which is `func` above), so we need to walk
         // upwards to find it.
         //
-        IRGlobalValue* valueForName = func;
+        IRInst* valueForName = func;
         for(;;)
         {
             auto parentBlock = as<IRBlock>(valueForName->parent);
@@ -3425,9 +3418,17 @@ struct EmitVisitor
             valueForName = parentGeneric;
         }
 
+        // If we reach this point, we are assuming that the value
+        // has some kind of linkage, and thus a mangled name.
+        //
+        auto linkageDecoration = valueForName->findDecoration<IRLinkageDecoration>();
+        SLANG_ASSERT(linkageDecoration);
+        auto mangledName = String(linkageDecoration->getMangledName());
+
+
         // We will use the `UnmangleContext` utility to
         // help us split the original name into its pieces.
-        UnmangleContext um(getText(valueForName->mangledName));
+        UnmangleContext um(mangledName);
         um.startUnmangling();
 
         // We'll read through the qualified name of the
@@ -5969,13 +5970,16 @@ struct EmitVisitor
             // appropriate decoration to these variables to indicate their
             // purpose.
             //
-            if(getText(varDecl->mangledName).StartsWith("gl_"))
+            if(auto linkageDecoration = varDecl->findDecoration<IRLinkageDecoration>())
             {
-                // The variable represents an OpenGL system value,
-                // so we will assume that it doesn't need to be declared.
-                //
-                // TODO: handle case where we *should* declare the variable.
-                return;
+                if(linkageDecoration->getMangledName().startsWith("gl_"))
+                {
+                    // The variable represents an OpenGL system value,
+                    // so we will assume that it doesn't need to be declared.
+                    //
+                    // TODO: handle case where we *should* declare the variable.
+                    return;
+                }
             }
         }
 
@@ -6363,6 +6367,31 @@ void legalizeTypes(
     TypeLegalizationContext*    context,
     IRModule*                   module);
 
+static void dumpIRIfEnabled(
+    CompileRequest* compileRequest,
+    IRModule*       irModule,
+    char const*     label = nullptr)
+{
+    if(compileRequest->shouldDumpIR)
+    {
+        WriterHelper writer(compileRequest->getWriter(WriterChannel::StdError));
+
+        if(label)
+        {
+            writer.put("### ");
+            writer.put(label);
+            writer.put(":\n");
+        }
+
+        dumpIR(irModule, writer.getWriter());
+
+        if( label )
+        {
+            writer.put("###\n");
+        }
+    }
+}
+
 String emitEntryPoint(
     EntryPointRequest*  entryPoint,
     ProgramLayout*      programLayout,
@@ -6422,22 +6451,15 @@ String emitEntryPoint(
             &sharedContext.extensionUsageTracker);
 
 #if 0
-        fprintf(stderr, "### CLONED:\n");
-        dumpIR(irModule);
-        fprintf(stderr, "###\n");
+        dumpIRIfEnabled(compileRequest, irModule, "CLONED");
 #endif
 
         validateIRModuleIfEnabled(compileRequest, irModule);
 
         // If the user specified the flag that they want us to dump
         // IR, then do it here, for the target-specific, but
         // un-specialized IR.
-        if (translationUnit->compileRequest->shouldDumpIR)
-        {
-            ISlangWriter* writer = translationUnit->compileRequest->getWriter(WriterChannel::StdError);
-
-            dumpIR(irModule, writer);
-        }
+        dumpIRIfEnabled(compileRequest, irModule);
 
         // Next, we need to ensure that the code we emit for
         // the target doesn't contain any operations that would
@@ -6449,9 +6471,7 @@ String emitEntryPoint(
 
         // Debugging code for IR transformations...
 #if 0
-        fprintf(stderr, "### SPECIALIZED:\n");
-        dumpIR(irModule);
-        fprintf(stderr, "###\n");
+        dumpIRIfEnabled(compileRequest, irModule, "SPECIALIZED");
 #endif
         validateIRModuleIfEnabled(compileRequest, irModule);
 
@@ -6466,9 +6486,7 @@ String emitEntryPoint(
 
         //  Debugging output of legalization
 #if 0
-        fprintf(stderr, "### LEGALIZED:\n");
-        dumpIR(irModule);
-        fprintf(stderr, "###\n");
+        dumpIRIfEnabled(compileRequest, irModule, "LEGALIZED");
 #endif
         validateIRModuleIfEnabled(compileRequest, irModule);
 
@@ -6480,9 +6498,7 @@ String emitEntryPoint(
         constructSSA(irModule);
 
 #if 0
-        fprintf(stderr, "### AFTER SSA:\n");
-        dumpIR(irModule);
-        fprintf(stderr, "###\n");
+        dumpIRIfEnabled(compileRequest, irModule, "AFTER SSA");
 #endif
         validateIRModuleIfEnabled(compileRequest, irModule);
 

source/slang/ir-constexpr.cpp

@@ -16,8 +16,8 @@ struct PropagateConstExprContext
     SharedIRBuilder sharedBuilder;
     IRBuilder builder;
 
-    List<IRGlobalValue*> workList;
-    HashSet<IRGlobalValue*> onWorkList;
+    List<IRInst*> workList;
+    HashSet<IRInst*> onWorkList;
 
     IRBuilder* getBuilder() { return &builder; }
 
@@ -158,7 +158,7 @@ bool propagateConstExprForward(
 
 void maybeAddToWorkList(
     PropagateConstExprContext*  context,
-    IRGlobalValue*              gv)
+    IRInst*                     gv)
 {
     if( !context->onWorkList.Contains(gv) )
     {
@@ -484,10 +484,7 @@ void propagateConstExpr(
 
     for( auto ii : module->getGlobalInsts() )
     {
-        auto gv = as<IRGlobalValue>(ii);
-        if (!gv)
-            continue;
-        maybeAddToWorkList(&context, gv);
+        maybeAddToWorkList(&context, ii);
     }
 
     // We will iterate applying propagation to one global value at a time

source/slang/ir-inst-defs.h

@@ -153,23 +153,19 @@ INST(StructType, struct, 0, PARENT)
 
 INST_RANGE(Type, VoidType, StructType)
 
-/*IRGlobalValue*/
+/*IRGlobalValueWithCode*/
+    /* IRGlobalValueWIthParams*/
+        INST(Func, func, 0, PARENT)
+        INST(Generic, generic, 0, PARENT)
+    INST_RANGE(GlobalValueWithParams, Func, Generic)
 
-    /*IRGlobalValueWithCode*/
-        /* IRGlobalValueWIthParams*/
-            INST(Func, func, 0, PARENT)
-            INST(Generic, generic, 0, PARENT)
-        INST_RANGE(GlobalValueWithParams, Func, Generic)
+    INST(GlobalVar, global_var, 0, 0)
+    INST(GlobalConstant, global_constant, 0, 0)
+INST_RANGE(GlobalValueWithCode, Func, GlobalConstant)
 
-        INST(GlobalVar, global_var, 0, 0)
-        INST(GlobalConstant, global_constant, 0, 0)
-    INST_RANGE(GlobalValueWithCode, Func, GlobalConstant)
-
-    INST(StructKey, key, 0, 0)
-    INST(GlobalGenericParam, global_generic_param, 0, 0)
-    INST(WitnessTable, witness_table, 0, 0)
-
-INST_RANGE(GlobalValue, StructType, WitnessTable)
+INST(StructKey, key, 0, 0)
+INST(GlobalGenericParam, global_generic_param, 0, 0)
+INST(WitnessTable, witness_table, 0, 0)
 
 INST(Module, module, 0, PARENT)
 
@@ -384,7 +380,12 @@ INST(HighLevelDeclDecoration,               highLevelDecl,          1, 0)
     INST(GloballyCoherentDecoration,        globallyCoherent,       0, 0)
     INST(PatchConstantFuncDecoration,       patchConstantFunc,      1, 0)
 
-INST_RANGE(Decoration, HighLevelDeclDecoration, PatchConstantFuncDecoration)
+    /* LinkageDecoration */
+        INST(ImportDecoration, import, 1, 0)
+        INST(ExportDecoration, export, 1, 0)
+    INST_RANGE(LinkageDecoration, ImportDecoration, ExportDecoration)
+
+INST_RANGE(Decoration, HighLevelDeclDecoration, ExportDecoration)
 
 
 //

source/slang/ir-insts.h

@@ -227,6 +227,37 @@ IR_SIMPLE_DECORATION(ReadNoneDecoration)
 IR_SIMPLE_DECORATION(EarlyDepthStencilDecoration)
 IR_SIMPLE_DECORATION(GloballyCoherentDecoration)
 
+    /// A decoration that marks a value as having linkage.
+    ///
+    /// A value with linkage is either exported from its module,
+    /// or will have a definition imported from another module.
+    /// In either case, it requires a mangled name to use when
+    /// matching imports and exports.
+    ///
+struct IRLinkageDecoration : IRDecoration
+{
+    IR_PARENT_ISA(LinkageDecoration)
+
+    IRStringLit* getMangledNameOperand() { return cast<IRStringLit>(getOperand(0)); }
+
+    UnownedStringSlice getMangledName()
+    {
+        return getMangledNameOperand()->getStringSlice();
+    }
+};
+
+struct IRImportDecoration : IRLinkageDecoration
+{
+    enum { kOp = kIROp_ImportDecoration };
+    IR_LEAF_ISA(ImportDecoration)
+};
+
+struct IRExportDecoration : IRLinkageDecoration
+{
+    enum { kOp = kIROp_ExportDecoration };
+    IR_LEAF_ISA(ExportDecoration)
+};
+
 // An instruction that specializes another IR value
 // (representing a generic) to a particular set of generic arguments 
 // (instructions representing types, witness tables, etc.)
@@ -544,7 +575,7 @@ struct IRWitnessTableEntry : IRInst
 // interface. It basically takes the form of a
 // map from the required members of the interface
 // to the IR values that satisfy those requirements.
-struct IRWitnessTable : IRGlobalValue
+struct IRWitnessTable : IRInst
 {
     IRInstList<IRWitnessTableEntry> getEntries()
     {
@@ -565,7 +596,7 @@ struct IRUndefined : IRInst
 
 // A global-scope generic parameter (a type parameter, a
 // constraint parameter, etc.)
-struct IRGlobalGenericParam : IRGlobalValue
+struct IRGlobalGenericParam : IRInst
 {
     IR_LEAF_ISA(GlobalGenericParam)
 };
@@ -797,10 +828,6 @@ struct IRBuilder
     IRType* getType(
         IROp            op);
 
-
-    IRWitnessTable* lookupWitnessTable(Name* mangledName);
-    void registerWitnessTable(IRWitnessTable* table);
-
         /// Create an empty basic block.
         ///
         /// The created block will not be inserted into the current
@@ -1043,6 +1070,15 @@ struct IRBuilder
         addDecoration(value, kIROp_PatchConstantFuncDecoration, patchConstantFunc);
     }
 
+    void addImportDecoration(IRInst* value, UnownedStringSlice const& mangledName)
+    {
+        addDecoration(value, kIROp_ImportDecoration, getStringValue(mangledName));
+    }
+
+    void addExportDecoration(IRInst* value, UnownedStringSlice const& mangledName)
+    {
+        addDecoration(value, kIROp_ExportDecoration, getStringValue(mangledName));
+    }
 };
 
 // Helper to establish the source location that will be used
@@ -1091,10 +1127,6 @@ IRSpecializationState* createIRSpecializationState(
 void destroyIRSpecializationState(IRSpecializationState* state);
 IRModule* getIRModule(IRSpecializationState* state);
 
-IRGlobalValue* getSpecializedGlobalValueForDeclRef(
-    IRSpecializationState*  state,
-    DeclRef<Decl> const&    declRef);
-
 struct ExtensionUsageTracker;
 
 // Clone the IR values reachable from the given entry point