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, 160 insertions, 199 deletions

diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp
index 81b554c3b..27eb1adfb 100644
--- a/source/slang/slang-ir.cpp
+++ b/source/slang/slang-ir.cpp
@@ -702,12 +702,8 @@ namespace Slang
         auto irModule = func->getModule();
         SLANG_ASSERT(irModule);
 
-        SharedIRBuilder sharedBuilder;
-        sharedBuilder.module = irModule;
-
-        IRBuilder builder;
-        builder.sharedBuilder = &sharedBuilder;
-
+        SharedIRBuilder sharedBuilder(irModule);
+        IRBuilder builder(sharedBuilder);
         builder.setInsertBefore(func);
 
         List<IRType*> paramTypes;
@@ -1121,16 +1117,33 @@ namespace Slang
 
     //
 
-    IRBlock* IRBuilder::getBlock()
+    IRInst* IRInsertLoc::getParent() const
+    {
+        auto inst = getInst();
+        switch(getMode())
+        {
+        default:
+        case Mode::None:
+            return nullptr;
+        case Mode::Before:
+        case Mode::After:
+            return inst->getParent();
+        case Mode::AtStart:
+        case Mode::AtEnd:
+            return inst;
+        }
+    }
+
+    IRBlock* IRInsertLoc::getBlock() const
     {
-        return as<IRBlock>(insertIntoParent);
+        return as<IRBlock>(getParent());
     }
 
     // Get the current function (or other value with code)
     // that we are inserting into (if any).
-    IRGlobalValueWithCode* IRBuilder::getFunc()
+    IRGlobalValueWithCode* IRInsertLoc::getFunc() const
     {
-        auto pp = insertIntoParent;
+        auto pp = getParent();
         if (auto block = as<IRBlock>(pp))
         {
             pp = pp->getParent();
@@ -1138,37 +1151,11 @@ namespace Slang
         return as<IRGlobalValueWithCode>(pp);
     }
 
-
-    void IRBuilder::setInsertInto(IRInst* insertInto)
-    {
-        insertIntoParent = insertInto;
-        insertBeforeInst = nullptr;
-    }
-
-    void IRBuilder::setInsertBefore(IRInst* insertBefore)
-    {
-        SLANG_ASSERT(insertBefore);
-        insertIntoParent = insertBefore->parent;
-        insertBeforeInst = insertBefore;
-    }
-
-
     // Add an instruction into the current scope
     void IRBuilder::addInst(
         IRInst*     inst)
     {
-        if(insertBeforeInst)
-        {
-            inst->insertBefore(insertBeforeInst);
-        }
-        else if (insertIntoParent)
-        {
-            inst->insertAtEnd(insertIntoParent);
-        }
-        else
-        {
-            // Don't append the instruction anywhere
-        }
+        inst->insertAt(m_insertLoc);
     }
 
     // Given two parent instructions, pick the better one to use as as
@@ -1313,33 +1300,34 @@ namespace Slang
         return parent;
     }
 
-    IRInst* createEmptyInst(
-        IRModule*   module,
-        IROp        op,
-        int         totalArgCount)
+    IRInst* IRModule::_allocateInst(
+        IROp    op,
+        Int     operandCount,
+        size_t  minSizeInBytes)
     {
-        size_t size = sizeof(IRInst) + (totalArgCount) * sizeof(IRUse);
-
-        SLANG_ASSERT(module);
-        IRInst* inst = (IRInst*)module->memoryArena.allocateAndZero(size);
-
-        inst->operandCount = uint32_t(totalArgCount);
-        inst->m_op = op;
-
-        return inst;
-    }
+        // There are two basic cases for instructions that affect how we compute size:
+        //
+        // * The default case is that an instruction's state is fully defined by the fields
+        //   in the `IRInst` base type, along with the trailing operand list (a tail-allocated
+        //   array of `IRUse`s. Almost all instructions need space allocated this way.
+        //
+        // * A small number of cases (currently `IRConstant`s and the `IRModule` type) have
+        //   *zero* operands but include additional state beyond the fields in `IRInst`.
+        //   For these cases we want to ensure that at least `sizeof(T)` bytes are allocated,
+        //   based on the specific leaf type `T`.
+        //
+        // We handle the combination of the two cases by just taking the maximum of the two
+        // different sizes.
+        //
+        size_t defaultSize = sizeof(IRInst) + (operandCount) * sizeof(IRUse);
+        size_t totalSize = minSizeInBytes > defaultSize ? minSizeInBytes : defaultSize;
 
-    IRInst* createEmptyInstWithSize(
-        IRModule*   module,
-        IROp        op,
-        size_t      totalSizeInBytes)
-    {
-        SLANG_ASSERT(totalSizeInBytes >= sizeof(IRInst));
+        IRInst* inst = (IRInst*) m_memoryArena.allocateAndZero(totalSize);
 
-        SLANG_ASSERT(module);
-        IRInst* inst = (IRInst*)module->memoryArena.allocateAndZero(totalSizeInBytes);
+        // TODO: Is it actually important to run a constructor here?
+        new(inst) IRInst();
 
-        inst->operandCount = 0;
+        inst->operandCount = uint32_t(operandCount);
         inst->m_op = op;
 
         return inst;
@@ -1561,14 +1549,10 @@ namespace Slang
         }
     }
 
-    static void maybeSetSourceLoc(
-        IRBuilder*  builder,
-        IRInst*     value)
+    void IRBuilder::_maybeSetSourceLoc(
+        IRInst*     inst)
     {
-        if(!builder)
-            return;
-
-        auto sourceLocInfo = builder->sourceLocInfo;
+        auto sourceLocInfo = getSourceLocInfo();
         if(!sourceLocInfo)
             return;
 
@@ -1584,7 +1568,7 @@ namespace Slang
             sourceLocInfo = sourceLocInfo->next;
         }
 
-        value->sourceLoc = sourceLocInfo->sourceLoc;
+        inst->sourceLoc = sourceLocInfo->sourceLoc;
     }
 
 #if SLANG_ENABLE_IR_BREAK_ALLOC
@@ -1606,56 +1590,39 @@ namespace Slang
     }
 #endif
 
-    // Create an IR instruction/value and initialize it.
-    //
-    // In this case `argCount` and `args` represent the
-    // arguments *after* the type (which is a mandatory
-    // argument for all instructions).
-    template<typename T>
-    static T* createInstImpl(
-        IRModule*       module,
-        IRBuilder*      builder,
-        IROp            op,
-        IRType*         type,
-        UInt            fixedArgCount,
-        IRInst* const* fixedArgs,
-        UInt                    varArgListCount,
-        UInt const*             listArgCounts,
+    IRInst* IRBuilder::_createInst(
+        size_t                  minSizeInBytes,
+        IRType*                 type,
+        IROp                    op,
+        Int                     fixedArgCount,
+        IRInst* const*          fixedArgs,
+        Int                     varArgListCount,
+        Int const*              listArgCounts,
         IRInst* const* const*   listArgs)
     {
-        UInt varArgCount = 0;
-        for (UInt ii = 0; ii < varArgListCount; ++ii)
+        Int varArgCount = 0;
+        for (Int ii = 0; ii < varArgListCount; ++ii)
         {
             varArgCount += listArgCounts[ii];
         }
 
-        UInt size = sizeof(IRInst) + (fixedArgCount + varArgCount) * sizeof(IRUse);
-        if (sizeof(T) > size)
-        {
-            size = sizeof(T);
-        }
+        Int totalOperandCount = fixedArgCount + varArgCount;
 
+        auto module = getModule();
         SLANG_ASSERT(module);
-        T* inst = (T*)module->memoryArena.allocateAndZero(size);
-
-        // TODO: Do we need to run ctor after zeroing?
-        new(inst)T();
+        IRInst* inst = module->_allocateInst(op, totalOperandCount, minSizeInBytes);
 
 #if SLANG_ENABLE_IR_BREAK_ALLOC
         inst->_debugUID = _debugGetAndIncreaseInstCounter();
 #endif
 
-        inst->operandCount = (uint32_t)(fixedArgCount + varArgCount);
-
-        inst->m_op = op;
-
         inst->typeUse.init(inst, type);
 
-        maybeSetSourceLoc(builder, inst);
+        _maybeSetSourceLoc(inst);
 
         auto operand = inst->getOperands();
 
-        for( UInt aa = 0; aa < fixedArgCount; ++aa )
+        for( Int aa = 0; aa < fixedArgCount; ++aa )
         {
             if (fixedArgs)
             {
@@ -1668,10 +1635,10 @@ namespace Slang
             operand++;
         }
 
-        for (UInt ii = 0; ii < varArgListCount; ++ii)
+        for (Int ii = 0; ii < varArgListCount; ++ii)
         {
-            UInt listArgCount = listArgCounts[ii];
-            for (UInt jj = 0; jj < listArgCount; ++jj)
+            Int listArgCount = listArgCounts[ii];
+            for (Int jj = 0; jj < listArgCount; ++jj)
             {
                 if (listArgs[ii])
                 {
@@ -1687,52 +1654,31 @@ namespace Slang
         return inst;
     }
 
-    static IRInst* createInstWithSizeImpl(
-        IRBuilder*      builder,
-        IROp            op,
-        IRType*         type,
-        size_t          sizeInBytes)
-    {
-        auto module = builder->getModule();
-        IRInst* inst = (IRInst*)module->memoryArena.allocate(sizeInBytes);
-        // Zero only the 'type'
-        memset(inst, 0, sizeof(IRInst));
-        // TODO: Do we need to run ctor after zeroing?
-        new (inst) IRInst;
-
-#if SLANG_ENABLE_IR_BREAK_ALLOC
-        inst->_debugUID = _debugGetAndIncreaseInstCounter();
-#endif
-
-        inst->m_op = op;
-        if (type)
-        {
-            inst->typeUse.init(inst, type);
-        }
-        maybeSetSourceLoc(builder, inst);
-        return inst; 
-    }
-
+    // Create an IR instruction/value and initialize it.
+    //
+    // In this case `argCount` and `args` represent the
+    // arguments *after* the type (which is a mandatory
+    // argument for all instructions).
     template<typename T>
     static T* createInstImpl(
-        IRBuilder*      builder,
-        IROp            op,
-        IRType*         type,
-        UInt            fixedArgCount,
-        IRInst* const* fixedArgs,
-        UInt           varArgCount = 0,
-        IRInst* const* varArgs = nullptr)
+        IRBuilder*              builder,
+        IROp                    op,
+        IRType*                 type,
+        Int                     fixedArgCount,
+        IRInst* const*          fixedArgs,
+        Int                     varArgListCount,
+        Int const*              listArgCounts,
+        IRInst* const* const*   listArgs)
     {
-        return createInstImpl<T>(
-            builder->getModule(),
-            builder,
-            op,
+        return (T*) builder->_createInst(
+            sizeof(T),
             type,
+            op,
             fixedArgCount,
             fixedArgs,
-            1,
-            &varArgCount,
-            &varArgs);
+            varArgListCount,
+            listArgCounts,
+            listArgs);
     }
 
     template<typename T>
@@ -1740,22 +1686,20 @@ namespace Slang
         IRBuilder*      builder,
         IROp            op,
         IRType*         type,
-        UInt            fixedArgCount,
+        Int             fixedArgCount,
         IRInst* const*  fixedArgs,
-        UInt                    varArgListCount,
-        UInt const*             listArgCount,
-        IRInst* const* const*   listArgs)
+        Int             varArgCount = 0,
+        IRInst* const*  varArgs = nullptr)
     {
         return createInstImpl<T>(
-            builder->getModule(),
             builder,
             op,
             type,
             fixedArgCount,
             fixedArgs,
-            varArgListCount,
-            listArgCount,
-            listArgs);
+            1,
+            &varArgCount,
+            &varArgs);
     }
 
     template<typename T>
@@ -1763,8 +1707,8 @@ namespace Slang
         IRBuilder*      builder,
         IROp            op,
         IRType*         type,
-        UInt            argCount,
-        IRInst* const* args)
+        Int             argCount,
+        IRInst* const*  args)
     {
         return createInstImpl<T>(
             builder,
@@ -1825,8 +1769,8 @@ namespace Slang
         IRBuilder*      builder,
         IROp            op,
         IRType*         type,
-        UInt            argCount,
-        IRInst* const* args)
+        Int             argCount,
+        IRInst* const*  args)
     {
         return createInstImpl<T>(
             builder,
@@ -1841,9 +1785,9 @@ namespace Slang
         IRBuilder*      builder,
         IROp            op,
         IRType*         type,
-        UInt            fixedArgCount,
+        Int             fixedArgCount,
         IRInst* const*  fixedArgs,
-        UInt            varArgCount,
+        Int             varArgCount,
         IRInst* const*  varArgs)
     {
         return createInstImpl<T>(
@@ -1862,7 +1806,7 @@ namespace Slang
         IROp            op,
         IRType*         type,
         IRInst*         arg1,
-        UInt            varArgCount,
+        Int             varArgCount,
         IRInst* const*  varArgs)
     {
         IRInst* fixedArgs[] = { arg1 };
@@ -2038,8 +1982,7 @@ namespace Slang
         }
     }
 
-    static IRConstant* findOrEmitConstant(
-        IRBuilder*      builder,
+    IRConstant* IRBuilder::_findOrEmitConstant(
         IRConstant&     keyInst)
     {
         // We now know where we want to insert, but there might
@@ -2054,7 +1997,7 @@ namespace Slang
         key.inst = &keyInst;
 
         IRConstant* irValue = nullptr;
-        if( builder->sharedBuilder->constantMap.TryGetValue(key, irValue) )
+        if( getSharedBuilder()->getConstantMap().TryGetValue(key, irValue) )
         {
             // We found a match, so just use that.
             return irValue;
@@ -2072,19 +2015,22 @@ namespace Slang
             case kIROp_BoolLit:
             case kIROp_IntLit:
             {
-                irValue = static_cast<IRConstant*>(createInstWithSizeImpl(builder, keyInst.getOp(), keyInst.getFullType(), prefixSize + sizeof(IRIntegerValue)));
+                const size_t instSize = prefixSize + sizeof(IRIntegerValue);
+                irValue = static_cast<IRConstant*>(_createInst(instSize, keyInst.getFullType(), keyInst.getOp()));
                 irValue->value.intVal = keyInst.value.intVal;
                 break; 
             }
             case kIROp_FloatLit:
             {
-                irValue = static_cast<IRConstant*>(createInstWithSizeImpl(builder, keyInst.getOp(), keyInst.getFullType(), prefixSize + sizeof(IRFloatingPointValue)));
+                const size_t instSize = prefixSize + sizeof(IRFloatingPointValue);
+                irValue = static_cast<IRConstant*>(_createInst(instSize, keyInst.getFullType(), keyInst.getOp()));
                 irValue->value.floatVal = keyInst.value.floatVal;
                 break;
             }
             case kIROp_PtrLit:
             {
-                irValue = static_cast<IRConstant*>(createInstWithSizeImpl(builder, keyInst.getOp(), keyInst.getFullType(), prefixSize + sizeof(void*)));
+                const size_t instSize = prefixSize + sizeof(void*);
+                irValue = static_cast<IRConstant*>(_createInst(instSize, keyInst.getFullType(), keyInst.getOp()));
                 irValue->value.ptrVal = keyInst.value.ptrVal;
                 break;
             }
@@ -2095,7 +2041,7 @@ namespace Slang
                 const size_t sliceSize = slice.getLength();
                 const size_t instSize = prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; 
 
-                irValue = static_cast<IRConstant*>(createInstWithSizeImpl(builder, keyInst.getOp(), keyInst.getFullType(), instSize));
+                irValue = static_cast<IRConstant*>(_createInst(instSize, keyInst.getFullType(), keyInst.getOp()));
 
                 IRConstant::StringValue& dstString = irValue->value.stringVal;
 
@@ -2110,9 +2056,9 @@ namespace Slang
         }
 
         key.inst = irValue;
-        builder->sharedBuilder->constantMap.Add(key, irValue);
+        getSharedBuilder()->getConstantMap().Add(key, irValue);
 
-        addHoistableInst(builder, irValue);
+        addHoistableInst(this, irValue);
 
         return irValue;
     }
@@ -2126,7 +2072,7 @@ namespace Slang
         keyInst.m_op = kIROp_BoolLit;
         keyInst.typeUse.usedValue = getBoolType();
         keyInst.value.intVal = IRIntegerValue(inValue);
-        return findOrEmitConstant(this, keyInst);
+        return _findOrEmitConstant(keyInst);
     }
 
     IRInst* IRBuilder::getIntValue(IRType* type, IRIntegerValue inValue)
@@ -2136,7 +2082,7 @@ namespace Slang
         keyInst.m_op = kIROp_IntLit;
         keyInst.typeUse.usedValue = type;
         keyInst.value.intVal = inValue;
-        return findOrEmitConstant(this, keyInst);
+        return _findOrEmitConstant(keyInst);
     }
 
     IRInst* IRBuilder::getFloatValue(IRType* type, IRFloatingPointValue inValue)
@@ -2146,7 +2092,7 @@ namespace Slang
         keyInst.m_op = kIROp_FloatLit;
         keyInst.typeUse.usedValue = type;
         keyInst.value.floatVal = inValue;
-        return findOrEmitConstant(this, keyInst);
+        return _findOrEmitConstant(keyInst);
     }
 
     IRStringLit* IRBuilder::getStringValue(const UnownedStringSlice& inSlice)
@@ -2167,7 +2113,7 @@ namespace Slang
         dstSlice.chars = const_cast<char*>(inSlice.begin());
         dstSlice.numChars = uint32_t(inSlice.getLength());
 
-        return static_cast<IRStringLit*>(findOrEmitConstant(this, keyInst));
+        return static_cast<IRStringLit*>(_findOrEmitConstant(keyInst));
     }
 
     IRPtrLit* IRBuilder::getPtrValue(void* value)
@@ -2179,7 +2125,7 @@ namespace Slang
         keyInst.m_op = kIROp_PtrLit;
         keyInst.typeUse.usedValue = type;
         keyInst.value.ptrVal = value;
-        return (IRPtrLit*) findOrEmitConstant(this, keyInst);
+        return (IRPtrLit*) _findOrEmitConstant(keyInst);
     }
 
     IRInst* IRBuilder::getCapabilityValue(CapabilitySet const& caps)
@@ -2226,7 +2172,7 @@ namespace Slang
             operandCount += listOperandCounts[ii];
         }
 
-        auto& memoryArena = getModule()->memoryArena;
+        auto& memoryArena = getModule()->getMemoryArena();
         void* cursor = memoryArena.getCursor();
 
         // We are going to create a 'dummy' instruction on the memoryArena
@@ -2266,7 +2212,7 @@ namespace Slang
             IRInstKey key = { inst };
 
             // Ideally we would add if not found, else return if was found instead of testing & then adding.
-            IRInst** found = sharedBuilder->globalValueNumberingMap.TryGetValueOrAdd(key, inst);
+            IRInst** found = getSharedBuilder()->getGlobalValueNumberingMap().TryGetValueOrAdd(key, inst);
             SLANG_ASSERT(endCursor == memoryArena.getCursor());
             // If it's found, just return, and throw away the instruction
             if (found)
@@ -2285,7 +2231,7 @@ namespace Slang
                 inst->typeUse.init(inst, type);
             }
 
-            maybeSetSourceLoc(this, inst);
+            _maybeSetSourceLoc(inst);
 
             IRUse*const operands = inst->getOperands();
             for (UInt i = 0; i < operandCount; ++i)
@@ -2316,7 +2262,7 @@ namespace Slang
             operandCount += listOperandCounts[ii];
         }
 
-        auto& memoryArena = getModule()->memoryArena;
+        auto& memoryArena = getModule()->getMemoryArena();
         void* cursor = memoryArena.getCursor();
 
         // We are going to create a 'dummy' instruction on the memoryArena
@@ -2356,7 +2302,7 @@ namespace Slang
             IRInstKey key = { inst };
 
             // Ideally we would add if not found, else return if was found instead of testing & then adding.
-            IRInst** found = sharedBuilder->globalValueNumberingMap.TryGetValueOrAdd(key, inst);
+            IRInst** found = getSharedBuilder()->getGlobalValueNumberingMap().TryGetValueOrAdd(key, inst);
             SLANG_ASSERT(endCursor == memoryArena.getCursor());
             // If it's found, just return, and throw away the instruction
             if (found)
@@ -2375,7 +2321,7 @@ namespace Slang
                 inst->typeUse.init(inst, type);
             }
 
-            maybeSetSourceLoc(this, inst);
+            _maybeSetSourceLoc(inst);
 
             IRUse*const operands = inst->getOperands();
             for (UInt i = 0; i < operandCount; ++i)
@@ -3216,22 +3162,13 @@ namespace Slang
         return inst;
     }
 
-    IRModule* IRBuilder::createModule()
+    RefPtr<IRModule> IRModule::create(Session* session)
     {
-        auto module = new IRModule();
-        module->session = getSession();
+        RefPtr<IRModule> module = new IRModule(session);
 
-        auto moduleInst = createInstImpl<IRModuleInst>(
-            module,
-            this,
-            kIROp_Module,
-            nullptr,
-            0,
-            nullptr,
-            0,
-            nullptr,
-            nullptr);
-        module->moduleInst = moduleInst;
+        auto moduleInst = module->_allocateInst<IRModuleInst>(kIROp_Module, 0);
+
+        module->m_moduleInst = moduleInst;
         moduleInst->module = module;
 
         return module;
@@ -3248,7 +3185,9 @@ namespace Slang
         // parent instruction for the builder, and
         // possibly work our way up.
         //
-        auto parent = builder->insertIntoParent;
+        auto defaultInsertLoc = builder->getInsertLoc();
+        auto defaultParent = defaultInsertLoc.getParent();
+        auto parent = defaultParent;
         while(parent)
         {
             // Inserting into the top level of a module?
@@ -3281,10 +3220,9 @@ namespace Slang
         // current "insert into" parent for the builder, then
         // we need to respect its "insert before" setting
         // as well.
-        if (parent == builder->insertIntoParent
-            && builder->insertBeforeInst)
+        if (parent == defaultParent)
         {
-            value->insertBefore(builder->insertBeforeInst);
+            value->insertAt(defaultInsertLoc);
         }
         else
         {
@@ -3298,7 +3236,7 @@ namespace Slang
             this,
             kIROp_Func,
             nullptr);
-        maybeSetSourceLoc(this, rsFunc);
+        _maybeSetSourceLoc(rsFunc);
         addGlobalValue(this, rsFunc);
         return rsFunc;
     }
@@ -3311,7 +3249,7 @@ namespace Slang
             this,
             kIROp_GlobalVar,
             ptrType);
-        maybeSetSourceLoc(this, globalVar);
+        _maybeSetSourceLoc(globalVar);
         addGlobalValue(this, globalVar);
         return globalVar;
     }
@@ -3323,7 +3261,7 @@ namespace Slang
             this,
             kIROp_GlobalParam,
             valueType);
-        maybeSetSourceLoc(this, inst);
+        _maybeSetSourceLoc(inst);
         addGlobalValue(this, inst);
         return inst;
     }
@@ -5581,6 +5519,29 @@ namespace Slang
         insertAtEnd(p);
     }
 
+    void IRInst::insertAt(IRInsertLoc const& loc)
+    {
+        removeFromParent();
+        IRInst* other = loc.getInst();
+        switch(loc.getMode())
+        {
+        case IRInsertLoc::Mode::None:
+            break;
+        case IRInsertLoc::Mode::Before:
+            insertBefore(other);
+            break;
+        case IRInsertLoc::Mode::After:
+            insertAfter(other);
+            break;
+        case IRInsertLoc::Mode::AtStart:
+            insertAtStart(other);
+            break;
+        case IRInsertLoc::Mode::AtEnd:
+            insertAtEnd(other);
+            break;
+        }
+    }
+
     // Remove this instruction from its parent block,
     // and then destroy it (it had better have no uses!)
     void IRInst::removeFromParent()