Cleanup refactoring work around the IR builder (#2061)

* Cleanup refactoring work around the IR builder

We have some long-term goals for the IR that require a more centralized and disciplined set of rules for how IR instructions get created/emitted. I had been working on trying to set things up so that all IR instruction creation goes through a single bottleneck point, but the non-trivial work in that branch was getting drowned out by the sheer volume of cleanup and refactoring changes. This change tries to pull together several of the more important cleanups.

The big pieces are:

* `IRBuilder` and `SharedIRBuilder` now protect their data members and rely on users to initialize them more directly via constructor of an `init()` method. This change affects a *bunch* of sites where `IRBuilder`s were created. I changed use sites to use the constructors whenever possible, and to use `init()` in cases where we had longer-lived builders that needed to be initialized multiple times.

* The insertion location for the `IRBuilder` now uses an encapsulated type called `IRInsertLoc`. This new type can replace what used to be just two `IRInst*` fields in the builder, and also covers some new functionality (if we ever want to take advantage of it). Very little client code cares about this change, but it is still a nice cleanup in terms of making things more explicit.

* The creation of an `IRModule` has been moded *out* of `IRBuilder`, because in practice we `IRBuilder` always wants to be associated with a pre-existing `IRModule` at creation time (via its `SharedIRBuilder`). There is now an `IRModule::create()` operation instead. This required changing the sequencing at many `IRModule` creation sites, since most had been contriving to make an `IRBuilder` first. There were also several cleanups because code had been carelessly using non-reference-counted pointers for `IRModule`s in ways that broke now that `IRModule::create()` always returns a `RefPtr`.

* The core operations to actually allocate memory for IR instructions were moved into `IRModule` (since they interact with the memory pool that the module owns). These *were* called `createEmptyInst()` but have been renamed into `_allocateInst()`. In principle these seem like they should only be needed to be called by the `IRBuilder`, but in practice they are also needed by the IR deserialization logic.

* A few core operations for emitting IR instructions that were associted with `IRBuilder` were moved to actually be methods on `IRBuilder`. First is `_findOrEmitConstant` which is the primary bottleneck for creating simple scalar constant values. Another is `_createInst` (formerly part of the templated `createInstImpl` along with `createInstWithSizeImpl`) which is the main bottleneck for allocation and initialization of any instruction other than a constant (well, the `IRModuleInst` is the other exception...). Finally, there is also `_maybeSetSourceLoc()`, which is obvious to scope inside the `IRBuilder` once it is protecting the source-location info.

Notes:

* The `minSizeInBytes` parameter to `_createInst()` might not actually be needed at all. At this point any `IRInst` subtypes that need data allocated for things other than their operands already get created manually via `_allocateInst` or `_findOrEmitConstant`, so I *think* we could remove that part. I will handle that in a subsequent cleanup if it turns out to be the case.

* There is one IR pass (`slang-ir-string-hash.cpp`) that is using manual `_allocateInst()` instead of going through an `IRBuilder`. It could be easily cleaned up to not do so (and I will probably make that change down the line), but for now I wanted to avoid doing anything that wasn't close to pure refactoring if I could.

* At this point in our design an `IRBuilder` is a very lightweight thing - it basically just owns the insertion location plus a source location to write into instructions. A lot of our code currently treats `IRBuilder`s like they are expensive and/or need to be re-used (which leads to them being used in more mutable/stateful ways). It is quite likely that as we clean up other aspects of the implementation of IR creation/emission we can make `IRBuilder` use feel more lightweight in ways that can streamline and simplify code.

* The next step for this work is to identify the different paths that eventually lead to `_createInst()` being called, and unify them at a single bottleneck operation that can own the decisions around when to create an instruction vs. when to re-use an existing one (rather than those decisions being baked into the various `IRBuilder` subroutines that create instructions of the various subtypes).

* fixup: gcc/clang C++ spec details

1 files changed, 58 insertions, 15 deletions

diff --git a/source/slang/slang-serialize-ir.cpp b/source/slang/slang-serialize-ir.cpp
index 96818a130..0824bd68d 100644
--- a/source/slang/slang-serialize-ir.cpp
+++ b/source/slang/slang-serialize-ir.cpp
@@ -676,16 +676,47 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
     typedef Ser::Inst::PayloadType PayloadType;
 
     m_serialData = &data;
- 
-    auto module = new IRModule();
+
+    auto module = IRModule::create(session);
     outModule = module;
     m_module = module;
 
-    module->session = session;
-
     // Convert m_stringTable into StringSlicePool.
     SerialStringTableUtil::decodeStringTable(data.m_stringTable.getBuffer(), data.m_stringTable.getCount(), m_stringTable);
 
+    // Each IR instruction has:
+    //
+    // * An opcode
+    // * Zero or more operands
+    // * Zero or more children
+    //
+    // Most instructions are entirely defined by those properties.
+    // 
+    // The instructions that represent simple constants (integers, strings, etc.) are
+    // unique in that they have "payload" data that holds their value, instead of having
+    // any operands.
+    //
+    // The deserialization logic here is set up to handle an arbitrary configuration
+    // of IR instructions, which means it can handle cases where:
+    //
+    // * An instruction earlier in the serialized stream might refer to an instruction
+    //   later in the stream, as one of its operands or (transitive) children.
+    //
+    // * An instruction in the stream transitively depends on itself via operand
+    //   and/or child relationships.
+    //
+    // In order to handle these cases, deserialization proceeds in multiple passes.
+    // In the first pass, `IRInst`s are allocated for each instruction in the stream,
+    // based on their memory requirements (number of operands in the ordinary case
+    // and payload size in the case of simple constants). Subsequent passes then
+    // fill in the operands and/or children.
+    //
+    // Note that as a result of the strategy used here, it is not possible for the
+    // deserialization logic to interact with any systems for deduplication or
+    // simplification of instructions. An alternative version of the deserializer that
+    // uses the `IRBuilder` interface instead might be possible, but would need a
+    // plan for how to handle forward and/or circular references in the IR module.
+
     // Add all the instructions
     List<IRInst*> insts;
 
@@ -704,10 +735,10 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
         SLANG_RELEASE_ASSERT(srcInst.m_op == kIROp_Module);
         SLANG_ASSERT(srcInst.m_payloadType == PayloadType::Empty);
 
-        // Create the module inst
-        auto moduleInst = static_cast<IRModuleInst*>(createEmptyInstWithSize(module, kIROp_Module, sizeof(IRModuleInst)));
-        module->moduleInst = moduleInst;
-        moduleInst->module = module;
+        // The root IR instruction for the module will already have
+        // been created as part of creating `module` above.
+        //
+        auto moduleInst = module->getModuleInst();
 
         // Set the IRModuleInst
         insts[1] = moduleInst; 
@@ -726,34 +757,41 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
             // Calculate the minimum object size (ie not including the payload of value)    
             const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value);
 
+            // All IR constants have zero operands.
+            Int operandCount = 0;
+
             IRConstant* irConst = nullptr;
             switch (op)
             {                    
                 case kIROp_BoolLit:
                 {
+                    // TODO: Most of these cases could use the templated `_allocateInst<T>`
+                    // *if* we had distinct `IRConstant` subtypes to represent these
+                    // cases and their subtype-specific payloads.
+
                     SLANG_ASSERT(srcInst.m_payloadType == PayloadType::UInt32);
-                    irConst = static_cast<IRConstant*>(createEmptyInstWithSize(module, op, prefixSize + sizeof(IRIntegerValue)));
+                    irConst = static_cast<IRConstant*>(module->_allocateInst(op, operandCount, prefixSize + sizeof(IRIntegerValue)));
                     irConst->value.intVal = srcInst.m_payload.m_uint32 != 0;
                     break;
                 }
                 case kIROp_IntLit:
                 {
                     SLANG_ASSERT(srcInst.m_payloadType == PayloadType::Int64);
-                    irConst = static_cast<IRConstant*>(createEmptyInstWithSize(module, op, prefixSize + sizeof(IRIntegerValue)));
+                    irConst = static_cast<IRConstant*>(module->_allocateInst(op, operandCount, prefixSize + sizeof(IRIntegerValue)));
                     irConst->value.intVal = srcInst.m_payload.m_int64; 
                     break;
                 }
                 case kIROp_PtrLit:
                 {
                     SLANG_ASSERT(srcInst.m_payloadType == PayloadType::Int64);
-                    irConst = static_cast<IRConstant*>(createEmptyInstWithSize(module, op, prefixSize + sizeof(void*)));
+                    irConst = static_cast<IRConstant*>(module->_allocateInst(op, operandCount, prefixSize + sizeof(void*)));
                     irConst->value.ptrVal = (void*) (intptr_t) srcInst.m_payload.m_int64; 
                     break;
                 }
                 case kIROp_FloatLit:
                 {
                     SLANG_ASSERT(srcInst.m_payloadType == PayloadType::Float64);
-                    irConst = static_cast<IRConstant*>(createEmptyInstWithSize(module, op,  prefixSize + sizeof(IRFloatingPointValue)));
+                    irConst = static_cast<IRConstant*>(module->_allocateInst(op, operandCount,  prefixSize + sizeof(IRFloatingPointValue)));
                     irConst->value.floatVal = srcInst.m_payload.m_float64;
                     break;
                 }
@@ -766,7 +804,7 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
                     const size_t sliceSize = slice.getLength();
                     const size_t instSize = prefixSize + SLANG_OFFSET_OF(IRConstant::StringValue, chars) + sliceSize;
 
-                    irConst = static_cast<IRConstant*>(createEmptyInstWithSize(module, op, instSize));
+                    irConst = static_cast<IRConstant*>(module->_allocateInst(op, operandCount, instSize));
 
                     IRConstant::StringValue& dstString = irConst->value.stringVal;
 
@@ -788,7 +826,12 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
         }
         else if (_isTextureTypeBase(op))
         {
-            IRTextureTypeBase* inst = static_cast<IRTextureTypeBase*>(createEmptyInst(module, op, 1));
+            // TODO: We should clean up the IR encoding of texture types so that
+            // they do not need to have special-case suport in the serialization layer.
+
+            // All IR texture types currently have a single operand
+            Int operandCount = 1;
+            IRTextureTypeBase* inst = module->_allocateInst<IRTextureTypeBase>(op, operandCount);
             SLANG_ASSERT(srcInst.m_payloadType == PayloadType::OperandAndUInt32);
 
             // Reintroduce the texture type bits into the the
@@ -800,7 +843,7 @@ Result IRSerialReader::read(const IRSerialData& data, Session* session, SerialSo
         else
         {
             int numOperands = srcInst.getNumOperands();
-            insts[i] = createEmptyInst(module, op, numOperands);
+            insts[i] = module->_allocateInst(op, numOperands);
         }
     }