From 024de9b86eb532b584e80555db01c3babb89903e Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Fri, 28 Feb 2025 14:34:29 -0800 Subject: [PATCH 01/20] WIP: Switch WitnessTable to HOISTABLE This commit deduplicates Witnessable by making it HOISTABLE. Three problems were observed when changed to HOISTABLE, and they are addressed in this commit. 1. A HOISTABLE IRInst is immutable once created. `SetOperand()` functions became unavailable. There were code that was modifying WitnessTable after creating it. This commit changed the behavior so that all operands are prepared before creating a new WitnessTable, and all operands are given to its constructor. 1. WitnessTable started to have duplicated decorations and children. Due to the nature of HOISTABLE, when create a new WitnessTable, a pre-existing WitnessTable is returned and reused. There isn't an easy way to tell if the WitnessTable is a brand new or reused at the moment. This commit assumes that it is a brand new only when it has no decorations and chilrend after its creation. 1. In `SimplifyIR()`, the order of children were slightly changed as a result of the optimization. The behavior was little different when WitnessTable became HOISTABLE. This resulted in an error where WitnessTable has a WitnessEntry pointing to an incorrect `IRSpecialize`. In order for it to function properly, `IRSpecialize` had to appear before `IRWitnessTable`. --- source/slang/slang-ir-autodiff.cpp | 27 +++- source/slang/slang-ir-inst-defs.h | 2 +- source/slang/slang-ir-insts.h | 2 - source/slang/slang-ir.cpp | 15 ++ source/slang/slang-lower-to-ir.cpp | 139 +++++++++++------- .../autodiff/deduplicate-witness-table.slang | 33 +++++ 6 files changed, 152 insertions(+), 66 deletions(-) create mode 100644 tests/autodiff/deduplicate-witness-table.slang diff --git a/source/slang/slang-ir-autodiff.cpp b/source/slang/slang-ir-autodiff.cpp index f3f32add2a..6d440d07db 100644 --- a/source/slang/slang-ir-autodiff.cpp +++ b/source/slang/slang-ir-autodiff.cpp @@ -3177,14 +3177,27 @@ struct AutoDiffPass : public InstPassBase List args; for (auto param : genType->getParams()) args.add(param); - as(innerResult.diffWitness) - ->setConcreteType((IRType*)builder.emitSpecializeInst( - builder.getTypeKind(), - originalType, - (UInt)args.getCount(), - args.getBuffer())); + + auto concreteType = as(builder.emitSpecializeInst( + builder.getTypeKind(), + originalType, + (UInt)args.getCount(), + args.getBuffer())); + + auto witnessTableType = innerResult.diffWitness->getFullType(); + auto newWitnessTable = builder.createWitnessTable(witnessTableType, concreteType); + + // Copy all entries from the old witness table to the new one + for (auto entry : as(innerResult.diffWitness)->getEntries()) + { + builder.createWitnessTableEntry( + newWitnessTable, + entry->getRequirementKey(), + entry->getSatisfyingVal()); + } + result.diffWitness = - hoistValueFromGeneric(builder, innerResult.diffWitness, specInst, true); + hoistValueFromGeneric(builder, newWitnessTable, specInst, true); } return result; } diff --git a/source/slang/slang-ir-inst-defs.h b/source/slang/slang-ir-inst-defs.h index 574f452433..8019fdd08d 100644 --- a/source/slang/slang-ir-inst-defs.h +++ b/source/slang/slang-ir-inst-defs.h @@ -294,7 +294,7 @@ INST(GlobalConstant, globalConstant, 0, GLOBAL) INST(StructKey, key, 0, GLOBAL) INST(GlobalGenericParam, global_generic_param, 0, GLOBAL) -INST(WitnessTable, witness_table, 0, 0) +INST(WitnessTable, witness_table, 0, HOISTABLE) INST(IndexedFieldKey, indexedFieldKey, 2, HOISTABLE) diff --git a/source/slang/slang-ir-insts.h b/source/slang/slang-ir-insts.h index fc8788b4d1..86596f3160 100644 --- a/source/slang/slang-ir-insts.h +++ b/source/slang/slang-ir-insts.h @@ -2980,8 +2980,6 @@ struct IRWitnessTable : IRInst IRType* getConcreteType() { return (IRType*)getOperand(0); } - void setConcreteType(IRType* t) { return setOperand(0, t); } - IR_LEAF_ISA(WitnessTable) }; diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 6d1d76afce..a129134ad9 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1694,6 +1694,21 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) insertBeforeInst = insertBeforeInst->getNextInst(); + if (inst->getOp() == kIROp_WitnessTable) + { + // WitnessTable may reference specialize inst-s from its WitnessEntry + // children. In this case, specialize insts must be cloned before the + // WitnessTable. + // + for (IRInst* iter = insertBeforeInst; iter; ) + { + bool isSpecialize = (iter->getOp() == kIROp_Specialize); + iter = iter->getNextInst(); + if (isSpecialize) + insertBeforeInst = iter; + } + } + // For instructions that will be placed at module scope, // we don't care about relative ordering, but for everything // else, we want to ensure that an instruction comes after diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index decfe4a913..d21feb63b6 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8045,27 +8045,38 @@ struct DeclLoweringVisitor : DeclVisitor irSatisfyingWitnessTable = subBuilder->createWitnessTable( irWitnessTableBaseType, irWitnessTable->getConcreteType()); - auto mangledName = getMangledNameForConformanceWitness( - subContext->astBuilder, - astReqWitnessTable->witnessedType, - astReqWitnessTable->baseType); - subBuilder->addExportDecoration( - irSatisfyingWitnessTable, - mangledName.getUnownedSlice()); - if (isExportedType(astReqWitnessTable->witnessedType)) + + // TODO: When WitnessTable became HOISTABLE, we needed a way to avoid + // adding the same decorations and childrens redundantly. + // There isn't an easy way to tell if the returned WitnessTable is a + // brand new or a found one from the existing pool. + // The code below assumes that when there are any decoration or child, + // it is a pre-existed one. + // + if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) { - subBuilder->addHLSLExportDecoration(irSatisfyingWitnessTable); - subBuilder->addKeepAliveDecoration(irSatisfyingWitnessTable); - } + auto mangledName = getMangledNameForConformanceWitness( + subContext->astBuilder, + astReqWitnessTable->witnessedType, + astReqWitnessTable->baseType); + subBuilder->addExportDecoration( + irSatisfyingWitnessTable, + mangledName.getUnownedSlice()); + if (isExportedType(astReqWitnessTable->witnessedType)) + { + subBuilder->addHLSLExportDecoration(irSatisfyingWitnessTable); + subBuilder->addKeepAliveDecoration(irSatisfyingWitnessTable); + } - // Recursively lower the sub-table. - lowerWitnessTable( - subContext, - astReqWitnessTable, - irSatisfyingWitnessTable, - mapASTToIRWitnessTable); + // Recursively lower the sub-table. + lowerWitnessTable( + subContext, + astReqWitnessTable, + irSatisfyingWitnessTable, + mapASTToIRWitnessTable); - irSatisfyingWitnessTable->moveToEnd(); + irSatisfyingWitnessTable->moveToEnd(); + } } irSatisfyingVal = irSatisfyingWitnessTable; } @@ -8174,59 +8185,75 @@ struct DeclLoweringVisitor : DeclVisitor // auto irWitnessTableBaseType = lowerType(subContext, superType); - // Create the IR-level witness table - auto irWitnessTable = subBuilder->createWitnessTable(irWitnessTableBaseType, nullptr); - - // Register the value now, rather than later, to avoid any possible infinite recursion. + // Register a dummy value to avoid infinite recursions. + // Without this, the call to lowerType() can get into an infinite recursion. + // context->setGlobalValue( inheritanceDecl, - LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); + LoweredValInfo::simple(findOuterMostGeneric(subBuilder->getInsertLoc().getParent()))); auto irSubType = lowerType(subContext, subType); - irWitnessTable->setConcreteType(irSubType); - // TODO(JS): - // Should the mangled name take part in obfuscation if enabled? + // Create the IR-level witness table + auto irWitnessTable = subBuilder->createWitnessTable(irWitnessTableBaseType, irSubType); + + // TODO: When WitnessTable became HOISTABLE, we needed a way to avoid + // adding the same decorations and childrens redundantly. + // There isn't an easy way to tell if the returned WitnessTable is a + // brand new or a found one from the existing pool. + // The code below assumes that when there are any decoration or child, + // it is a pre-existed one. + // + if (irWitnessTable->getFirstDecorationOrChild() == nullptr) + { + // Override with the correct witness-table + context->setGlobalValue( + inheritanceDecl, + LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); - addLinkageDecoration( - context, - irWitnessTable, - inheritanceDecl, - mangledName.getUnownedSlice()); + // TODO(JS): + // Should the mangled name take part in obfuscation if enabled? - // If the witness table is for a COM interface, always keep it alive. - if (irWitnessTableBaseType->findDecoration()) - { - subBuilder->addHLSLExportDecoration(irWitnessTable); - } + addLinkageDecoration( + context, + irWitnessTable, + inheritanceDecl, + mangledName.getUnownedSlice()); - for (auto mod : parentDecl->modifiers) - { - if (as(mod)) + // If the witness table is for a COM interface, always keep it alive. + if (irWitnessTableBaseType->findDecoration()) { subBuilder->addHLSLExportDecoration(irWitnessTable); - subBuilder->addKeepAliveDecoration(irWitnessTable); } - else if (as(mod)) + + for (auto mod : parentDecl->modifiers) { - subBuilder->addAutoDiffBuiltinDecoration(irWitnessTable); + if (as(mod)) + { + subBuilder->addHLSLExportDecoration(irWitnessTable); + subBuilder->addKeepAliveDecoration(irWitnessTable); + } + else if (as(mod)) + { + subBuilder->addAutoDiffBuiltinDecoration(irWitnessTable); + } } - } - // Make sure that all the entries in the witness table have been filled in, - // including any cases where there are sub-witness-tables for conformances - bool isExplicitExtern = false; - bool isImported = isImportedDecl(context, parentDecl, isExplicitExtern); - if (!isImported || isExplicitExtern) - { - Dictionary mapASTToIRWitnessTable; - lowerWitnessTable( - subContext, - inheritanceDecl->witnessTable, - irWitnessTable, - mapASTToIRWitnessTable); + // Make sure that all the entries in the witness table have been filled in, + // including any cases where there are sub-witness-tables for conformances + bool isExplicitExtern = false; + bool isImported = isImportedDecl(context, parentDecl, isExplicitExtern); + if (!isImported || isExplicitExtern) + { + Dictionary mapASTToIRWitnessTable; + lowerWitnessTable( + subContext, + inheritanceDecl->witnessTable, + irWitnessTable, + mapASTToIRWitnessTable); + } + irWitnessTable->moveToEnd(); } - irWitnessTable->moveToEnd(); return LoweredValInfo::simple( finishOuterGenerics(subBuilder, irWitnessTable, outerGeneric)); diff --git a/tests/autodiff/deduplicate-witness-table.slang b/tests/autodiff/deduplicate-witness-table.slang new file mode 100644 index 0000000000..ea4c4e7308 --- /dev/null +++ b/tests/autodiff/deduplicate-witness-table.slang @@ -0,0 +1,33 @@ +//TEST:SIMPLE(filecheck=CHK):-stage compute -entry computeMain -target hlsl + +//CHK: struct DiffPair_1 +//CHK-NOT: struct DiffPair_2 + +RWTexture2D gOutputColor; + +struct ShadingFrame : IDifferentiable +{ + float3 T; +} + +[Differentiable] +float computeRay() +{ + float3 dir = 1.f; + return dot(dir, dir); +} + +[Differentiable] +float paramRay() +{ + DifferentialPair dpDir = fwd_diff(computeRay)(); + return dpDir.p; +} + +[Shader("compute")] +[NumThreads(1, 1, 1)] +void computeMain(int3 dispatchThreadID : SV_DispatchThreadID) +{ + DifferentialPair dpColor = fwd_diff(paramRay)(); + gOutputColor[0] = dpColor.p; +} From 2265771925318979dc2dd6c0499ff00102a98919 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Tue, 4 Mar 2025 05:24:34 -0800 Subject: [PATCH 02/20] Fix a test although it seems to break another --- source/slang/slang-ir-specialize.cpp | 47 +++++++++++++++++++++++++++- source/slang/slang-ir.cpp | 15 --------- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index a200a907b0..97cc5ee925 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3096,7 +3096,52 @@ IRInst* specializeGenericImpl( // instructions only, because parameters were dealt // with explictly at an earlier point. // - for (auto ii : bb->getOrdinaryInsts()) + IRInstList ordinaryInsts = bb->getOrdinaryInsts(); + + // A block can have an IRWitnessTable that refers to an IRSpecialize in the same block. In + // this case, the IRSpecialize must be cloned before the cloning of the IRWitnessTable, + // because the operands are expected to be cloned prior to the cloning of the IRInst. + // + // Similarly, there can be an IRWitnessTable that refers to antoehr IRWitnessTable in the + // same block. + // + // In order to resolve the problem, we need to iterate the ordinary insts and change the + // order to resolve the dependencies. + // + List insts; + int instCount = 0; + for (IRInst* oi : ordinaryInsts) + { + SLANG_UNUSED(oi); + instCount++; + } + insts.reserve(instCount); + + for (IRInst* oi : ordinaryInsts) + { + int whereToAdd = 0; + for (; whereToAdd < insts.getCount(); whereToAdd++) + { + IRInst* sibling = insts[whereToAdd]; + + // Figure out if there are dependencies. + if (auto wtSibling = as(sibling)) + { + for (auto entry : wtSibling->getEntries()) + { + if (entry->getRequirementKey() == oi || entry->getSatisfyingVal() == oi) + { + goto dependencyFound; + } + } + } + } + + dependencyFound: + insts.insert(whereToAdd, oi); + } + + for (auto ii : insts) { // The last block of the generic is expected to end with // a `return` instruction for the specialized value that diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index a129134ad9..6d1d76afce 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1694,21 +1694,6 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) insertBeforeInst = insertBeforeInst->getNextInst(); - if (inst->getOp() == kIROp_WitnessTable) - { - // WitnessTable may reference specialize inst-s from its WitnessEntry - // children. In this case, specialize insts must be cloned before the - // WitnessTable. - // - for (IRInst* iter = insertBeforeInst; iter; ) - { - bool isSpecialize = (iter->getOp() == kIROp_Specialize); - iter = iter->getNextInst(); - if (isSpecialize) - insertBeforeInst = iter; - } - } - // For instructions that will be placed at module scope, // we don't care about relative ordering, but for everything // else, we want to ensure that an instruction comes after From b4f4ec52d581dc84b39dfa4d22a5856ee2e13575 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Tue, 4 Mar 2025 14:27:14 -0800 Subject: [PATCH 03/20] Fix one more failing test. --- source/slang/slang-ir-specialize.cpp | 84 +++++++++++++++++++--------- 1 file changed, 59 insertions(+), 25 deletions(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 97cc5ee925..263ca1c146 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3029,6 +3029,31 @@ void finalizeSpecialization(IRModule* module) } } +// Returns true when "a" depends on "b". +bool IsIRInstDependOn(IRInst *a, IRInst *b) +{ + if (a->getFullType() == b) + return true; + + for (int i = 0; i < a->getOperandCount(); i++) + { + auto operand = a->getOperand(i); + if (operand == b) + return true; + } + + if (auto wt = as(a)) + { + for (auto entry : wt->getEntries()) + { + if (entry->getRequirementKey() == b || entry->getSatisfyingVal() == b) + return true; + } + } + + return false; +} + IRInst* specializeGenericImpl( IRGeneric* genericVal, IRSpecialize* specializeInst, @@ -3098,47 +3123,56 @@ IRInst* specializeGenericImpl( // IRInstList ordinaryInsts = bb->getOrdinaryInsts(); - // A block can have an IRWitnessTable that refers to an IRSpecialize in the same block. In - // this case, the IRSpecialize must be cloned before the cloning of the IRWitnessTable, - // because the operands are expected to be cloned prior to the cloning of the IRInst. - // - // Similarly, there can be an IRWitnessTable that refers to antoehr IRWitnessTable in the - // same block. - // - // In order to resolve the problem, we need to iterate the ordinary insts and change the - // order to resolve the dependencies. + // After IRWitnessTable became Hoistable, they are removed and insered back by + // `addHoistableInst()`. But when they are re-inserted, the order it appears in the block is + // changed. When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be + // cloned before the cloning of IRWitnessTable. It is because the operands are assumed to be + // cloned before the cloning of IRInst. + // + // We need to resolve the dependency problem by changing the order of them. // List insts; int instCount = 0; - for (IRInst* oi : ordinaryInsts) + for (IRInst* ii : ordinaryInsts) { - SLANG_UNUSED(oi); + SLANG_UNUSED(ii); instCount++; } insts.reserve(instCount); - for (IRInst* oi : ordinaryInsts) + // First, add all instructions in their original order + for (IRInst* ii : ordinaryInsts) + { + insts.add(ii); + } + + // We'll need multiple passes to handle complex dependency chains + bool madeChanges = true; + while (madeChanges) { - int whereToAdd = 0; - for (; whereToAdd < insts.getCount(); whereToAdd++) + madeChanges = false; + + for (int i = 0; i < instCount; i++) { - IRInst* sibling = insts[whereToAdd]; + IRInst* current = insts[i]; - // Figure out if there are dependencies. - if (auto wtSibling = as(sibling)) + // Find all instructions that `current` depends on + for (int j = i + 1; j < instCount; j++) { - for (auto entry : wtSibling->getEntries()) + IRInst* later = insts[j]; + + if (IsIRInstDependOn(current, later)) { - if (entry->getRequirementKey() == oi || entry->getSatisfyingVal() == oi) - { - goto dependencyFound; - } + // `current` depends on `later`, so `later` must come before `current`. + insts.removeAt(j); + insts.insert(i, later); + + // mark the change, and continue to the next. + madeChanges = true; + i++; } } } - - dependencyFound: - insts.insert(whereToAdd, oi); } for (auto ii : insts) From d5283055bfbadad8dbb0aefe13e4bf89c98be521 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Wed, 5 Mar 2025 11:13:12 -0800 Subject: [PATCH 04/20] Prevent WitnessTables from being hoisted Fixes the failing test, tests/compute/assoctype-func-param.slang --- source/slang/slang-ir-specialize.cpp | 2 +- source/slang/slang-ir.cpp | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 263ca1c146..37fc1121c4 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3030,7 +3030,7 @@ void finalizeSpecialization(IRModule* module) } // Returns true when "a" depends on "b". -bool IsIRInstDependOn(IRInst *a, IRInst *b) +bool IsIRInstDependOn(IRInst* a, IRInst* b) { if (a->getFullType() == b) return true; diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 6d1d76afce..5638b1282e 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -7985,7 +7985,11 @@ static void _replaceInstUsesWith(IRInst* thisInst, IRInst* other) auto user = uu->getUser(); bool userIsHoistable = getIROpInfo(user->getOp()).isHoistable(); - if (userIsHoistable) + + // We want to de-duplicate WitnessTable but we don't really want to hoist them. + bool userNeedToBeHoisted = userIsHoistable && (user->getOp() != kIROp_WitnessTable); + + if (userNeedToBeHoisted) { if (!dedupContext) { @@ -8002,7 +8006,7 @@ static void _replaceInstUsesWith(IRInst* thisInst, IRInst* other) // to a point before `user`, if it is not already so. _maybeHoistOperand(uu); - if (userIsHoistable) + if (userNeedToBeHoisted) { // Is the updated inst already exists in the global numbering map? // If so, we need to continue work on replacing the updated inst with the existing From 8189082b6b25207fbb7c825cfa3a63d27cf2539d Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Wed, 5 Mar 2025 18:58:05 -0800 Subject: [PATCH 05/20] Update the comments --- source/slang/slang-ir-specialize.cpp | 11 ++++++++--- source/slang/slang-lower-to-ir.cpp | 18 ++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 37fc1121c4..e78cfbd5c9 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3123,13 +3123,18 @@ IRInst* specializeGenericImpl( // IRInstList ordinaryInsts = bb->getOrdinaryInsts(); - // After IRWitnessTable became Hoistable, they are removed and insered back by + // After IRWitnessTable became Hoistable, they are removed and inserted back by // `addHoistableInst()`. But when they are re-inserted, the order it appears in the block is - // changed. When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be + // changed. We need to change the order in a way that the dependancy is resolved. + // + // The dependency cannot be resolve in `addHoistableInst()`, because IRWitnessTable doesn't + // have IRWitnessTableEntry yet while in the function. + // + // When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be // cloned before the cloning of IRWitnessTable. It is because the operands are assumed to be // cloned before the cloning of IRInst. // - // We need to resolve the dependency problem by changing the order of them. + // Similarly, there can be dependencies between an IRWitnessTable and another IRWitnessTable. // List insts; int instCount = 0; diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index d21feb63b6..bb8472aac0 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8046,12 +8046,9 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTableBaseType, irWitnessTable->getConcreteType()); - // TODO: When WitnessTable became HOISTABLE, we needed a way to avoid - // adding the same decorations and childrens redundantly. - // There isn't an easy way to tell if the returned WitnessTable is a - // brand new or a found one from the existing pool. - // The code below assumes that when there are any decoration or child, - // it is a pre-existed one. + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return a + // pre-existing one. We need to avoid adding the same decorations/children + // when the IRWitnessTable already has them. // if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) { @@ -8197,12 +8194,9 @@ struct DeclLoweringVisitor : DeclVisitor // Create the IR-level witness table auto irWitnessTable = subBuilder->createWitnessTable(irWitnessTableBaseType, irSubType); - // TODO: When WitnessTable became HOISTABLE, we needed a way to avoid - // adding the same decorations and childrens redundantly. - // There isn't an easy way to tell if the returned WitnessTable is a - // brand new or a found one from the existing pool. - // The code below assumes that when there are any decoration or child, - // it is a pre-existed one. + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return a + // pre-existing one. We need to avoid adding the same decorations/children + // when the IRWitnessTable already has them. // if (irWitnessTable->getFirstDecorationOrChild() == nullptr) { From 2b17660c0dc55469731192605ca5c03d660ddcde Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 6 Mar 2025 07:45:48 -0800 Subject: [PATCH 06/20] Clean up in autodiff --- source/slang/slang-ir-autodiff.cpp | 226 +++++++++++++++------------ source/slang/slang-ir-link.cpp | 1 + source/slang/slang-ir-specialize.cpp | 11 +- source/slang/slang-lower-to-ir.cpp | 30 ++-- 4 files changed, 146 insertions(+), 122 deletions(-) diff --git a/source/slang/slang-ir-autodiff.cpp b/source/slang/slang-ir-autodiff.cpp index 6d440d07db..ad94212570 100644 --- a/source/slang/slang-ir-autodiff.cpp +++ b/source/slang/slang-ir-autodiff.cpp @@ -1862,17 +1862,20 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( sharedContext->differentiableInterfaceType, (IRType*)pairType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeStructKey, - diffDiffPairType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeWitnessStructKey, - table); - builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); - builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeStructKey, + diffDiffPairType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeWitnessStructKey, + table); + builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); + builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + } bool isUserCodeType = as(pairType) ? true : false; @@ -1944,15 +1947,18 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)pairType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeStructKey, - diffDiffPairType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeWitnessStructKey, - table); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeStructKey, + diffDiffPairType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeWitnessStructKey, + table); + } } return table; @@ -1987,17 +1993,20 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( sharedContext->differentiableInterfaceType, (IRType*)arrayType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeStructKey, - diffArrayType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeWitnessStructKey, - table); - builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); - builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeStructKey, + diffArrayType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeWitnessStructKey, + table); + builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); + builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + } auto elementType = as(diffArrayType)->getElementType(); @@ -2066,15 +2075,18 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)arrayType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeStructKey, - diffArrayType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeWitnessStructKey, - table); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeStructKey, + diffArrayType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeWitnessStructKey, + table); + } } else { @@ -2105,17 +2117,20 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( sharedContext->differentiableInterfaceType, (IRType*)inTupleType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeStructKey, - diffTupleType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocTypeWitnessStructKey, - table); - builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); - builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeStructKey, + diffTupleType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocTypeWitnessStructKey, + table); + builder->createWitnessTableEntry(table, sharedContext->addMethodStructKey, addMethod); + builder->createWitnessTableEntry(table, sharedContext->zeroMethodStructKey, zeroMethod); + } // Fill in differential method implementations. { @@ -2219,15 +2234,18 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)inTupleType); - // And place it in the synthesized witness table. - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeStructKey, - diffTupleType); - builder->createWitnessTableEntry( - table, - sharedContext->differentialAssocRefTypeWitnessStructKey, - table); + if (table->getFirstDecorationOrChild() == nullptr) + { + // And place it in the synthesized witness table. + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeStructKey, + diffTupleType); + builder->createWitnessTableEntry( + table, + sharedContext->differentialAssocRefTypeWitnessStructKey, + table); + } } return table; @@ -3078,39 +3096,45 @@ struct AutoDiffPass : public InstPassBase builder.createWitnessTable(autodiffContext->differentiableInterfaceType, originalType); result.diffWitness = origTypeIsDiffWitness; - builder.createWitnessTableEntry( - origTypeIsDiffWitness, - autodiffContext->differentialAssocTypeStructKey, - diffType); - builder.createWitnessTableEntry( - origTypeIsDiffWitness, - autodiffContext->differentialAssocTypeWitnessStructKey, - diffTypeIsDiffWitness); - builder.createWitnessTableEntry( - origTypeIsDiffWitness, - autodiffContext->zeroMethodStructKey, - zeroMethod); - builder.createWitnessTableEntry( - origTypeIsDiffWitness, - autodiffContext->addMethodStructKey, - addMethod); - - builder.createWitnessTableEntry( - diffTypeIsDiffWitness, - autodiffContext->differentialAssocTypeStructKey, - diffType); - builder.createWitnessTableEntry( - diffTypeIsDiffWitness, - autodiffContext->differentialAssocTypeWitnessStructKey, - diffTypeIsDiffWitness); - builder.createWitnessTableEntry( - diffTypeIsDiffWitness, - autodiffContext->zeroMethodStructKey, - zeroMethod); - builder.createWitnessTableEntry( - diffTypeIsDiffWitness, - autodiffContext->addMethodStructKey, - addMethod); + if (origTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) + { + builder.createWitnessTableEntry( + origTypeIsDiffWitness, + autodiffContext->differentialAssocTypeStructKey, + diffType); + builder.createWitnessTableEntry( + origTypeIsDiffWitness, + autodiffContext->differentialAssocTypeWitnessStructKey, + diffTypeIsDiffWitness); + builder.createWitnessTableEntry( + origTypeIsDiffWitness, + autodiffContext->zeroMethodStructKey, + zeroMethod); + builder.createWitnessTableEntry( + origTypeIsDiffWitness, + autodiffContext->addMethodStructKey, + addMethod); + } + + if (diffTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) + { + builder.createWitnessTableEntry( + diffTypeIsDiffWitness, + autodiffContext->differentialAssocTypeStructKey, + diffType); + builder.createWitnessTableEntry( + diffTypeIsDiffWitness, + autodiffContext->differentialAssocTypeWitnessStructKey, + diffTypeIsDiffWitness); + builder.createWitnessTableEntry( + diffTypeIsDiffWitness, + autodiffContext->zeroMethodStructKey, + zeroMethod); + builder.createWitnessTableEntry( + diffTypeIsDiffWitness, + autodiffContext->addMethodStructKey, + addMethod); + } return result; } @@ -3178,6 +3202,7 @@ struct AutoDiffPass : public InstPassBase for (auto param : genType->getParams()) args.add(param); + // Create a new WitnessTable with a different concreteType. auto concreteType = as(builder.emitSpecializeInst( builder.getTypeKind(), originalType, @@ -3187,13 +3212,16 @@ struct AutoDiffPass : public InstPassBase auto witnessTableType = innerResult.diffWitness->getFullType(); auto newWitnessTable = builder.createWitnessTable(witnessTableType, concreteType); - // Copy all entries from the old witness table to the new one - for (auto entry : as(innerResult.diffWitness)->getEntries()) + if (newWitnessTable->getFirstDecorationOrChild() == nullptr) { - builder.createWitnessTableEntry( - newWitnessTable, - entry->getRequirementKey(), - entry->getSatisfyingVal()); + builder.setInsertInto(newWitnessTable); + for (auto entry : as(innerResult.diffWitness)->getEntries()) + { + builder.createWitnessTableEntry( + newWitnessTable, + entry->getRequirementKey(), + entry->getSatisfyingVal()); + } } result.diffWitness = diff --git a/source/slang/slang-ir-link.cpp b/source/slang/slang-ir-link.cpp index 125ab71d0d..4646dc9d15 100644 --- a/source/slang/slang-ir-link.cpp +++ b/source/slang/slang-ir-link.cpp @@ -732,6 +732,7 @@ IRWitnessTable* cloneWitnessTableImpl( clonedBaseType = cloneType(context, (IRType*)(originalTable->getConformanceType())); auto clonedSubType = cloneType(context, (IRType*)(originalTable->getConcreteType())); clonedTable = builder->createWitnessTable(clonedBaseType, clonedSubType); + SLANG_RELEASE_ASSERT(clonedTable->getFirstDecorationOrChild() == nullptr); } else { diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index e78cfbd5c9..37fc1121c4 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3123,18 +3123,13 @@ IRInst* specializeGenericImpl( // IRInstList ordinaryInsts = bb->getOrdinaryInsts(); - // After IRWitnessTable became Hoistable, they are removed and inserted back by + // After IRWitnessTable became Hoistable, they are removed and insered back by // `addHoistableInst()`. But when they are re-inserted, the order it appears in the block is - // changed. We need to change the order in a way that the dependancy is resolved. - // - // The dependency cannot be resolve in `addHoistableInst()`, because IRWitnessTable doesn't - // have IRWitnessTableEntry yet while in the function. - // - // When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be + // changed. When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be // cloned before the cloning of IRWitnessTable. It is because the operands are assumed to be // cloned before the cloning of IRInst. // - // Similarly, there can be dependencies between an IRWitnessTable and another IRWitnessTable. + // We need to resolve the dependency problem by changing the order of them. // List insts; int instCount = 0; diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index bb8472aac0..08966f65ce 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8046,10 +8046,9 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTableBaseType, irWitnessTable->getConcreteType()); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return a - // pre-existing one. We need to avoid adding the same decorations/children - // when the IRWitnessTable already has them. - // + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return an + // IRWitnessTable that already has decorations/children. We need to avoid + // adding them more than once. if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) { auto mangledName = getMangledNameForConformanceWitness( @@ -8071,9 +8070,9 @@ struct DeclLoweringVisitor : DeclVisitor astReqWitnessTable, irSatisfyingWitnessTable, mapASTToIRWitnessTable); - - irSatisfyingWitnessTable->moveToEnd(); } + + irSatisfyingWitnessTable->moveToEnd(); } irSatisfyingVal = irSatisfyingWitnessTable; } @@ -8194,17 +8193,17 @@ struct DeclLoweringVisitor : DeclVisitor // Create the IR-level witness table auto irWitnessTable = subBuilder->createWitnessTable(irWitnessTableBaseType, irSubType); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return a - // pre-existing one. We need to avoid adding the same decorations/children - // when the IRWitnessTable already has them. + // Override with the correct witness-table + context->setGlobalValue( + inheritanceDecl, + LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); + + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return an + // IRWitnessTable that already has decorations/children. We need to avoid adding them + // more than once. // if (irWitnessTable->getFirstDecorationOrChild() == nullptr) { - // Override with the correct witness-table - context->setGlobalValue( - inheritanceDecl, - LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); - // TODO(JS): // Should the mangled name take part in obfuscation if enabled? @@ -8246,9 +8245,10 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTable, mapASTToIRWitnessTable); } - irWitnessTable->moveToEnd(); } + irWitnessTable->moveToEnd(); + return LoweredValInfo::simple( finishOuterGenerics(subBuilder, irWitnessTable, outerGeneric)); } From a7474de9b2e4a546e8b3dab02e2cd6a474f52a31 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 6 Mar 2025 09:17:51 -0800 Subject: [PATCH 07/20] Fix compiler warning, which is treated as an error --- source/slang/slang-ir-specialize.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 37fc1121c4..79fed29dac 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3035,7 +3035,7 @@ bool IsIRInstDependOn(IRInst* a, IRInst* b) if (a->getFullType() == b) return true; - for (int i = 0; i < a->getOperandCount(); i++) + for (UInt i = 0; i < a->getOperandCount(); i++) { auto operand = a->getOperand(i); if (operand == b) From 12bdad2a4f811136042fb394b7de90666bbe9a88 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 6 Mar 2025 10:37:51 -0800 Subject: [PATCH 08/20] Fix the order changing issue --- source/slang/slang-ir-specialize.cpp | 56 +--------------------------- source/slang/slang-ir.cpp | 34 +++++++++++++++++ source/slang/slang-lower-to-ir.cpp | 8 ++-- 3 files changed, 39 insertions(+), 59 deletions(-) diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 79fed29dac..14e1154377 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3121,61 +3121,7 @@ IRInst* specializeGenericImpl( // instructions only, because parameters were dealt // with explictly at an earlier point. // - IRInstList ordinaryInsts = bb->getOrdinaryInsts(); - - // After IRWitnessTable became Hoistable, they are removed and insered back by - // `addHoistableInst()`. But when they are re-inserted, the order it appears in the block is - // changed. When IRWitnessTable refers to IRSpecialize, as an example, IRSpecialize must be - // cloned before the cloning of IRWitnessTable. It is because the operands are assumed to be - // cloned before the cloning of IRInst. - // - // We need to resolve the dependency problem by changing the order of them. - // - List insts; - int instCount = 0; - for (IRInst* ii : ordinaryInsts) - { - SLANG_UNUSED(ii); - instCount++; - } - insts.reserve(instCount); - - // First, add all instructions in their original order - for (IRInst* ii : ordinaryInsts) - { - insts.add(ii); - } - - // We'll need multiple passes to handle complex dependency chains - bool madeChanges = true; - while (madeChanges) - { - madeChanges = false; - - for (int i = 0; i < instCount; i++) - { - IRInst* current = insts[i]; - - // Find all instructions that `current` depends on - for (int j = i + 1; j < instCount; j++) - { - IRInst* later = insts[j]; - - if (IsIRInstDependOn(current, later)) - { - // `current` depends on `later`, so `later` must come before `current`. - insts.removeAt(j); - insts.insert(i, later); - - // mark the change, and continue to the next. - madeChanges = true; - i++; - } - } - } - } - - for (auto ii : insts) + for (auto ii : bb->getOrdinaryInsts()) { // The last block of the generic is expected to end with // a `return` instruction for the specialized value that diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 5638b1282e..ba3a6807fb 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1694,6 +1694,30 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) insertBeforeInst = insertBeforeInst->getNextInst(); + if (inst->getOp() == kIROp_WitnessTable) + { + // WitnessTable can refer to IRSpecialize from its WitnessTableEntry + // children. In this case, specialize insts must be cloned before the + // WitnessTable. Similar an WitnessTables can have depdency to another + // WitnessTable. + // + for (IRInst* iter = insertBeforeInst; iter;) + { + bool mayHaveDependency = false; + switch (iter->getOp()) + { + case kIROp_Specialize: + case kIROp_WitnessTable: + mayHaveDependency = true; + break; + } + + iter = iter->getNextInst(); + if (mayHaveDependency) + insertBeforeInst = iter; + } + } + // For instructions that will be placed at module scope, // we don't care about relative ordering, but for everything // else, we want to ensure that an instruction comes after @@ -4621,6 +4645,16 @@ void addGlobalValue(IRBuilder* builder, IRInst* value) parent = builder->getModule()->getModuleInst(); } + // If the value is already in the parent, keep it as it. Because WitnessTable is Hoistable, the + // parent can have only one instance of this WitnessTable. The order among siblings should + // remain because the later siblings may have dependency to the earlier siblings. + // + if (parent == value->parent) + { + SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); + return; + } + // If it turns out that we are inserting into the // current "insert into" parent for the builder, then // we need to respect its "insert before" setting diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index 08966f65ce..2aaab3e413 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8070,9 +8070,9 @@ struct DeclLoweringVisitor : DeclVisitor astReqWitnessTable, irSatisfyingWitnessTable, mapASTToIRWitnessTable); - } - irSatisfyingWitnessTable->moveToEnd(); + irSatisfyingWitnessTable->moveToEnd(); + } } irSatisfyingVal = irSatisfyingWitnessTable; } @@ -8245,9 +8245,9 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTable, mapASTToIRWitnessTable); } - } - irWitnessTable->moveToEnd(); + irWitnessTable->moveToEnd(); + } return LoweredValInfo::simple( finishOuterGenerics(subBuilder, irWitnessTable, outerGeneric)); From 166a85cf52c294229a56a4ee9f7b74cf0d954be0 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 6 Mar 2025 10:45:46 -0800 Subject: [PATCH 09/20] Update comments --- source/slang/slang-ir-autodiff.cpp | 9 +++++++++ source/slang/slang-ir-specialize.cpp | 25 ------------------------- source/slang/slang-ir.cpp | 11 ++++++----- source/slang/slang-lower-to-ir.cpp | 13 +++++++------ 4 files changed, 22 insertions(+), 36 deletions(-) diff --git a/source/slang/slang-ir-autodiff.cpp b/source/slang/slang-ir-autodiff.cpp index ad94212570..c1806cd00b 100644 --- a/source/slang/slang-ir-autodiff.cpp +++ b/source/slang/slang-ir-autodiff.cpp @@ -1862,6 +1862,7 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( sharedContext->differentiableInterfaceType, (IRType*)pairType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -1947,6 +1948,7 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)pairType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -1993,6 +1995,7 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( sharedContext->differentiableInterfaceType, (IRType*)arrayType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -2075,6 +2078,7 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)arrayType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -2117,6 +2121,7 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( sharedContext->differentiableInterfaceType, (IRType*)inTupleType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -2234,6 +2239,7 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( sharedContext->differentiablePtrInterfaceType, (IRType*)inTupleType); + // Add WitnessTableEntry only once if (table->getFirstDecorationOrChild() == nullptr) { // And place it in the synthesized witness table. @@ -3096,6 +3102,7 @@ struct AutoDiffPass : public InstPassBase builder.createWitnessTable(autodiffContext->differentiableInterfaceType, originalType); result.diffWitness = origTypeIsDiffWitness; + // Add WitnessTableEntry only once if (origTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) { builder.createWitnessTableEntry( @@ -3116,6 +3123,7 @@ struct AutoDiffPass : public InstPassBase addMethod); } + // Add WitnessTableEntry only once if (diffTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) { builder.createWitnessTableEntry( @@ -3212,6 +3220,7 @@ struct AutoDiffPass : public InstPassBase auto witnessTableType = innerResult.diffWitness->getFullType(); auto newWitnessTable = builder.createWitnessTable(witnessTableType, concreteType); + // Add WitnessTableEntry only once if (newWitnessTable->getFirstDecorationOrChild() == nullptr) { builder.setInsertInto(newWitnessTable); diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp index 14e1154377..a200a907b0 100644 --- a/source/slang/slang-ir-specialize.cpp +++ b/source/slang/slang-ir-specialize.cpp @@ -3029,31 +3029,6 @@ void finalizeSpecialization(IRModule* module) } } -// Returns true when "a" depends on "b". -bool IsIRInstDependOn(IRInst* a, IRInst* b) -{ - if (a->getFullType() == b) - return true; - - for (UInt i = 0; i < a->getOperandCount(); i++) - { - auto operand = a->getOperand(i); - if (operand == b) - return true; - } - - if (auto wt = as(a)) - { - for (auto entry : wt->getEntries()) - { - if (entry->getRequirementKey() == b || entry->getSatisfyingVal() == b) - return true; - } - } - - return false; -} - IRInst* specializeGenericImpl( IRGeneric* genericVal, IRSpecialize* specializeInst, diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index ba3a6807fb..2fcc8c9795 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1696,10 +1696,11 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) if (inst->getOp() == kIROp_WitnessTable) { - // WitnessTable can refer to IRSpecialize from its WitnessTableEntry - // children. In this case, specialize insts must be cloned before the - // WitnessTable. Similar an WitnessTables can have depdency to another - // WitnessTable. + SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); + + // WitnessTable can refer to IRSpecialize from its WitnessTableEntry children. In this case, + // specialize insts must be cloned before the WitnessTable. Similar an WitnessTables can + // have depdency to another WitnessTable. // for (IRInst* iter = insertBeforeInst; iter;) { @@ -4645,7 +4646,7 @@ void addGlobalValue(IRBuilder* builder, IRInst* value) parent = builder->getModule()->getModuleInst(); } - // If the value is already in the parent, keep it as it. Because WitnessTable is Hoistable, the + // If the value is already in the parent, keep it as-is. Because WitnessTable is Hoistable, the // parent can have only one instance of this WitnessTable. The order among siblings should // remain because the later siblings may have dependency to the earlier siblings. // diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index 2aaab3e413..4452785117 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8046,9 +8046,10 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTableBaseType, irWitnessTable->getConcreteType()); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return an - // IRWitnessTable that already has decorations/children. We need to avoid - // adding them more than once. + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return + // an IRWitnessTable that already has decorations/children. We should + // add them only once. + // if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) { auto mangledName = getMangledNameForConformanceWitness( @@ -8198,9 +8199,9 @@ struct DeclLoweringVisitor : DeclVisitor inheritanceDecl, LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return an - // IRWitnessTable that already has decorations/children. We need to avoid adding them - // more than once. + // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return + // an IRWitnessTable that already has decorations/children. We should + // add them only once. // if (irWitnessTable->getFirstDecorationOrChild() == nullptr) { From 0ce4975fe7916f7a2adb312d4547cb0b95166f5c Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 6 Mar 2025 18:56:07 -0800 Subject: [PATCH 10/20] Remove the sorting logic --- source/slang/slang-ir.cpp | 52 ++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 2fcc8c9795..523a88e769 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1687,35 +1687,29 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) // IRInst* insertBeforeInst = parent->getFirstChild(); - // Hoistable instructions are always "ordinary" - // instructions, so they need to come after - // any parameters of the parent. - // - while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) - insertBeforeInst = insertBeforeInst->getNextInst(); - if (inst->getOp() == kIROp_WitnessTable) { SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); - // WitnessTable can refer to IRSpecialize from its WitnessTableEntry children. In this case, - // specialize insts must be cloned before the WitnessTable. Similar an WitnessTables can - // have depdency to another WitnessTable. + // Because IRWitnessTable is Hoistable, do not move when + // it is already a child of the parent. // - for (IRInst* iter = insertBeforeInst; iter;) - { - bool mayHaveDependency = false; - switch (iter->getOp()) - { - case kIROp_Specialize: - case kIROp_WitnessTable: - mayHaveDependency = true; - break; - } + if (parent == inst->parent) + return; - iter = iter->getNextInst(); - if (mayHaveDependency) - insertBeforeInst = iter; + // For the rest of the cases, IRWitnessTable goes to the + // end of the list. + insertBeforeInst = nullptr; + } + else + { + // Hoistable instructions are always "ordinary" + // instructions, so they need to come after + // any parameters of the parent. + // + while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) + { + insertBeforeInst = insertBeforeInst->getNextInst(); } } @@ -4646,11 +4640,13 @@ void addGlobalValue(IRBuilder* builder, IRInst* value) parent = builder->getModule()->getModuleInst(); } - // If the value is already in the parent, keep it as-is. Because WitnessTable is Hoistable, the - // parent can have only one instance of this WitnessTable. The order among siblings should - // remain because the later siblings may have dependency to the earlier siblings. - // - if (parent == value->parent) + // If the value is already in the parent, keep it as-is. + // Because WitnessTable is Hoistable, the parent can have + // only one instance of this WitnessTable. The order among + // siblings should remain because the later siblings may + // have dependency to the earlier siblings. + // + if (parent == value->parent && value->getOp() == kIROp_WitnessTable) { SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); return; From f084dc7af4f027552f5670fb60b324c8496d17a7 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Fri, 7 Mar 2025 01:41:23 -0800 Subject: [PATCH 11/20] Fix some of failing Falcor tests --- source/slang/slang-ir-link.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/slang/slang-ir-link.cpp b/source/slang/slang-ir-link.cpp index 4646dc9d15..e1c03f1a58 100644 --- a/source/slang/slang-ir-link.cpp +++ b/source/slang/slang-ir-link.cpp @@ -732,7 +732,8 @@ IRWitnessTable* cloneWitnessTableImpl( clonedBaseType = cloneType(context, (IRType*)(originalTable->getConformanceType())); auto clonedSubType = cloneType(context, (IRType*)(originalTable->getConcreteType())); clonedTable = builder->createWitnessTable(clonedBaseType, clonedSubType); - SLANG_RELEASE_ASSERT(clonedTable->getFirstDecorationOrChild() == nullptr); + if (clonedTable->getFirstDecorationOrChild() != nullptr) + return clonedTable; } else { From 9c1a750ee5003bef5520067173042026947c03e1 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Sun, 9 Mar 2025 19:01:56 -0700 Subject: [PATCH 12/20] Fix two mistakes --- source/slang/slang-ir-autodiff.cpp | 6 ++++-- source/slang/slang-ir.cpp | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/slang/slang-ir-autodiff.cpp b/source/slang/slang-ir-autodiff.cpp index c1806cd00b..b8bf790859 100644 --- a/source/slang/slang-ir-autodiff.cpp +++ b/source/slang/slang-ir-autodiff.cpp @@ -3217,8 +3217,10 @@ struct AutoDiffPass : public InstPassBase (UInt)args.getCount(), args.getBuffer())); - auto witnessTableType = innerResult.diffWitness->getFullType(); - auto newWitnessTable = builder.createWitnessTable(witnessTableType, concreteType); + auto witnessTableType = + cast(innerResult.diffWitness->getFullType()); + auto conformanceType = cast(witnessTableType->getConformanceType()); + auto newWitnessTable = builder.createWitnessTable(conformanceType, concreteType); // Add WitnessTableEntry only once if (newWitnessTable->getFirstDecorationOrChild() == nullptr) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 523a88e769..867fc8220a 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1698,8 +1698,11 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) return; // For the rest of the cases, IRWitnessTable goes to the - // end of the list. - insertBeforeInst = nullptr; + // end of the list but before the terminators. + while (insertBeforeInst && !as(insertBeforeInst)) + { + insertBeforeInst = insertBeforeInst->getNextInst(); + } } else { From 01f27b185647b7169961c5d36d5baf5596c145aa Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Fri, 14 Mar 2025 11:27:19 -0700 Subject: [PATCH 13/20] Fix the issues with enum export/import --- source/slang/slang-lower-to-ir.cpp | 33 ++++++++++++++-------------- source/slang/slang-mangle.cpp | 35 ++++++++++++++++++++++++++++++ source/slang/slang-mangle.h | 5 +++++ 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index 4452785117..2e96968d0b 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8046,16 +8046,15 @@ struct DeclLoweringVisitor : DeclVisitor irWitnessTableBaseType, irWitnessTable->getConcreteType()); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return - // an IRWitnessTable that already has decorations/children. We should - // add them only once. - // + // Avoid adding same decorations and child more than once. if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) { auto mangledName = getMangledNameForConformanceWitness( subContext->astBuilder, astReqWitnessTable->witnessedType, - astReqWitnessTable->baseType); + astReqWitnessTable->baseType, + irWitnessTable->getConcreteType()); + subBuilder->addExportDecoration( irSatisfyingWitnessTable, mangledName.getUnownedSlice()); @@ -8156,14 +8155,6 @@ struct DeclLoweringVisitor : DeclVisitor } } - // Construct the mangled name for the witness table, which depends - // on the type that is conforming, and the type that it conforms to. - // - // TODO: This approach doesn't really make sense for generic `extension` conformances. - auto mangledName = - getMangledNameForConformanceWitness(context->astBuilder, subType, superType); - - // A witness table may need to be generic, if the outer // declaration (either a type declaration or an `extension`) // is generic. @@ -8199,12 +8190,20 @@ struct DeclLoweringVisitor : DeclVisitor inheritanceDecl, LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); - // Since IRWitnessTable is Hoistable, `createWitnessTable()` may return - // an IRWitnessTable that already has decorations/children. We should - // add them only once. - // + // Avoid adding same decorations and child more than once. if (irWitnessTable->getFirstDecorationOrChild() == nullptr) { + // Construct the mangled name for the witness table, which depends + // on the type that is conforming, and the type that it conforms to. + // + // TODO: This approach doesn't really make sense for generic `extension` + // conformances. + auto mangledName = getMangledNameForConformanceWitness( + context->astBuilder, + subType, + superType, + irSubType); + // TODO(JS): // Should the mangled name take part in obfuscation if enabled? diff --git a/source/slang/slang-mangle.cpp b/source/slang/slang-mangle.cpp index d51fafb6bd..6cb0d012a6 100644 --- a/source/slang/slang-mangle.cpp +++ b/source/slang/slang-mangle.cpp @@ -824,6 +824,41 @@ String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Ty return context.sb.produceString(); } +// This function takes an additional parameter to get a simplified +// mangled name when the witness-table is for enum-type. +// +// In order to deduplicate the witness-tables, we need to apply a little different +// rule for the mangled name when the `superType` is `enum` type. +// All witness-table for enum types whose underlying type is same should get the same +// manged name. +// +// TODO: We should remove this function and have a new IR for enum-type. The "option 2" +// described on the issue 6364 is more proper and ideal solution for the issue. +// +String getMangledNameForConformanceWitness( + ASTBuilder* astBuilder, + Type* sub, + Type* sup, + IRType* irSubType) +{ + SLANG_AST_BUILDER_RAII(astBuilder); + + ManglingContext context(astBuilder); + emitRaw(&context, "_SW"); + + if (ASTNodeType(sup->getClassInfo().m_classId) == ASTNodeType::EnumTypeType) + { + emitRaw(&context, getIROpInfo(irSubType->getOp()).name); + } + else + { + emitType(&context, sub); + } + + emitType(&context, sup); + return context.sb.produceString(); +} + String getMangledTypeName(ASTBuilder* astBuilder, Type* type) { SLANG_AST_BUILDER_RAII(astBuilder); diff --git a/source/slang/slang-mangle.h b/source/slang/slang-mangle.h index cfbe4fb25b..a7175f91bd 100644 --- a/source/slang/slang-mangle.h +++ b/source/slang/slang-mangle.h @@ -17,6 +17,11 @@ String getMangledNameFromNameString(const UnownedStringSlice& name); String getHashedName(const UnownedStringSlice& mangledName); String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Type* sup); +String getMangledNameForConformanceWitness( + ASTBuilder* astBuilder, + Type* sub, + Type* sup, + IRType* irSubType); String getMangledNameForConformanceWitness( ASTBuilder* astBuilder, DeclRef sub, From 7864b8b9e9b1535652a97bd5e687e80a7c27f51d Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Sat, 15 Mar 2025 13:14:10 -0700 Subject: [PATCH 14/20] Small clean up --- source/slang/slang-lower-to-ir.cpp | 13 ++++++++----- source/slang/slang-mangle.cpp | 8 ++------ source/slang/slang-mangle.h | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index 2e96968d0b..601b06872d 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8042,9 +8042,11 @@ struct DeclLoweringVisitor : DeclVisitor // Need to construct a sub-witness-table auto irWitnessTableBaseType = lowerType(subContext, astReqWitnessTable->baseType); - irSatisfyingWitnessTable = subBuilder->createWitnessTable( - irWitnessTableBaseType, - irWitnessTable->getConcreteType()); + + auto concreteType = irWitnessTable->getConcreteType(); + + irSatisfyingWitnessTable = + subBuilder->createWitnessTable(irWitnessTableBaseType, concreteType); // Avoid adding same decorations and child more than once. if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) @@ -8053,11 +8055,12 @@ struct DeclLoweringVisitor : DeclVisitor subContext->astBuilder, astReqWitnessTable->witnessedType, astReqWitnessTable->baseType, - irWitnessTable->getConcreteType()); + concreteType->getOp()); subBuilder->addExportDecoration( irSatisfyingWitnessTable, mangledName.getUnownedSlice()); + if (isExportedType(astReqWitnessTable->witnessedType)) { subBuilder->addHLSLExportDecoration(irSatisfyingWitnessTable); @@ -8202,7 +8205,7 @@ struct DeclLoweringVisitor : DeclVisitor context->astBuilder, subType, superType, - irSubType); + irSubType->getOp()); // TODO(JS): // Should the mangled name take part in obfuscation if enabled? diff --git a/source/slang/slang-mangle.cpp b/source/slang/slang-mangle.cpp index 6cb0d012a6..7899c3e736 100644 --- a/source/slang/slang-mangle.cpp +++ b/source/slang/slang-mangle.cpp @@ -835,11 +835,7 @@ String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Ty // TODO: We should remove this function and have a new IR for enum-type. The "option 2" // described on the issue 6364 is more proper and ideal solution for the issue. // -String getMangledNameForConformanceWitness( - ASTBuilder* astBuilder, - Type* sub, - Type* sup, - IRType* irSubType) +String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Type* sup, IROp subOp) { SLANG_AST_BUILDER_RAII(astBuilder); @@ -848,7 +844,7 @@ String getMangledNameForConformanceWitness( if (ASTNodeType(sup->getClassInfo().m_classId) == ASTNodeType::EnumTypeType) { - emitRaw(&context, getIROpInfo(irSubType->getOp()).name); + emitRaw(&context, getIROpInfo(subOp).name); } else { diff --git a/source/slang/slang-mangle.h b/source/slang/slang-mangle.h index a7175f91bd..cfdbe461b0 100644 --- a/source/slang/slang-mangle.h +++ b/source/slang/slang-mangle.h @@ -21,7 +21,7 @@ String getMangledNameForConformanceWitness( ASTBuilder* astBuilder, Type* sub, Type* sup, - IRType* irSubType); + IROp subOp); String getMangledNameForConformanceWitness( ASTBuilder* astBuilder, DeclRef sub, From 28f9adfd47a47fe6ba9af42672375dd43f0aed90 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Tue, 18 Mar 2025 06:10:09 -0700 Subject: [PATCH 15/20] Minor clean up based on the feedback --- source/slang/slang-ir-autodiff.cpp | 18 +++++++++--------- source/slang/slang-ir-link.cpp | 2 +- source/slang/slang-ir.h | 1 + source/slang/slang-lower-to-ir.cpp | 4 ++-- source/slang/slang-mangle.cpp | 2 +- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/source/slang/slang-ir-autodiff.cpp b/source/slang/slang-ir-autodiff.cpp index b8bf790859..afd698e8b6 100644 --- a/source/slang/slang-ir-autodiff.cpp +++ b/source/slang/slang-ir-autodiff.cpp @@ -1863,7 +1863,7 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( (IRType*)pairType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -1949,7 +1949,7 @@ IRInst* DifferentiableTypeConformanceContext::buildDifferentiablePairWitness( (IRType*)pairType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -1996,7 +1996,7 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( (IRType*)arrayType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -2079,7 +2079,7 @@ IRInst* DifferentiableTypeConformanceContext::buildArrayWitness( (IRType*)arrayType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -2122,7 +2122,7 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( (IRType*)inTupleType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -2240,7 +2240,7 @@ IRInst* DifferentiableTypeConformanceContext::buildTupleWitness( (IRType*)inTupleType); // Add WitnessTableEntry only once - if (table->getFirstDecorationOrChild() == nullptr) + if (!table->hasDecorationOrChild()) { // And place it in the synthesized witness table. builder->createWitnessTableEntry( @@ -3103,7 +3103,7 @@ struct AutoDiffPass : public InstPassBase result.diffWitness = origTypeIsDiffWitness; // Add WitnessTableEntry only once - if (origTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) + if (!origTypeIsDiffWitness->hasDecorationOrChild()) { builder.createWitnessTableEntry( origTypeIsDiffWitness, @@ -3124,7 +3124,7 @@ struct AutoDiffPass : public InstPassBase } // Add WitnessTableEntry only once - if (diffTypeIsDiffWitness->getFirstDecorationOrChild() == nullptr) + if (!diffTypeIsDiffWitness->hasDecorationOrChild()) { builder.createWitnessTableEntry( diffTypeIsDiffWitness, @@ -3223,7 +3223,7 @@ struct AutoDiffPass : public InstPassBase auto newWitnessTable = builder.createWitnessTable(conformanceType, concreteType); // Add WitnessTableEntry only once - if (newWitnessTable->getFirstDecorationOrChild() == nullptr) + if (!newWitnessTable->hasDecorationOrChild()) { builder.setInsertInto(newWitnessTable); for (auto entry : as(innerResult.diffWitness)->getEntries()) diff --git a/source/slang/slang-ir-link.cpp b/source/slang/slang-ir-link.cpp index e1c03f1a58..a84390f14a 100644 --- a/source/slang/slang-ir-link.cpp +++ b/source/slang/slang-ir-link.cpp @@ -732,7 +732,7 @@ IRWitnessTable* cloneWitnessTableImpl( clonedBaseType = cloneType(context, (IRType*)(originalTable->getConformanceType())); auto clonedSubType = cloneType(context, (IRType*)(originalTable->getConcreteType())); clonedTable = builder->createWitnessTable(clonedBaseType, clonedSubType); - if (clonedTable->getFirstDecorationOrChild() != nullptr) + if (clonedTable->hasDecorationOrChild()) return clonedTable; } else diff --git a/source/slang/slang-ir.h b/source/slang/slang-ir.h index 64125be9a2..dbc66c6a37 100644 --- a/source/slang/slang-ir.h +++ b/source/slang/slang-ir.h @@ -690,6 +690,7 @@ struct IRInst m_decorationsAndChildren.last); } void removeAndDeallocateAllDecorationsAndChildren(); + bool hasDecorationOrChild() { return m_decorationsAndChildren.first != nullptr; } #ifdef SLANG_ENABLE_IR_BREAK_ALLOC // Unique allocation ID for this instruction since start of current process. diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp index 601b06872d..929bc074bd 100644 --- a/source/slang/slang-lower-to-ir.cpp +++ b/source/slang/slang-lower-to-ir.cpp @@ -8049,7 +8049,7 @@ struct DeclLoweringVisitor : DeclVisitor subBuilder->createWitnessTable(irWitnessTableBaseType, concreteType); // Avoid adding same decorations and child more than once. - if (irSatisfyingWitnessTable->getFirstDecorationOrChild() == nullptr) + if (!irSatisfyingWitnessTable->hasDecorationOrChild()) { auto mangledName = getMangledNameForConformanceWitness( subContext->astBuilder, @@ -8194,7 +8194,7 @@ struct DeclLoweringVisitor : DeclVisitor LoweredValInfo::simple(findOuterMostGeneric(irWitnessTable))); // Avoid adding same decorations and child more than once. - if (irWitnessTable->getFirstDecorationOrChild() == nullptr) + if (!irWitnessTable->hasDecorationOrChild()) { // Construct the mangled name for the witness table, which depends // on the type that is conforming, and the type that it conforms to. diff --git a/source/slang/slang-mangle.cpp b/source/slang/slang-mangle.cpp index 7899c3e736..838abcfc53 100644 --- a/source/slang/slang-mangle.cpp +++ b/source/slang/slang-mangle.cpp @@ -842,7 +842,7 @@ String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Ty ManglingContext context(astBuilder); emitRaw(&context, "_SW"); - if (ASTNodeType(sup->getClassInfo().m_classId) == ASTNodeType::EnumTypeType) + if (isDeclRefTypeOf(sup)) { emitRaw(&context, getIROpInfo(subOp).name); } From 11b6d912ef0d83959fffd7d05f4430a552a3d353 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Tue, 18 Mar 2025 23:36:35 -0700 Subject: [PATCH 16/20] Fix the incorrect use of isDeclRefTypeOf --- source/slang/slang-mangle.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/slang/slang-mangle.cpp b/source/slang/slang-mangle.cpp index 838abcfc53..12e185c8bd 100644 --- a/source/slang/slang-mangle.cpp +++ b/source/slang/slang-mangle.cpp @@ -842,7 +842,7 @@ String getMangledNameForConformanceWitness(ASTBuilder* astBuilder, Type* sub, Ty ManglingContext context(astBuilder); emitRaw(&context, "_SW"); - if (isDeclRefTypeOf(sup)) + if (as(sup)) { emitRaw(&context, getIROpInfo(subOp).name); } From 37deff751ef029cdc3e62014c0b914a5df79cf26 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Wed, 19 Mar 2025 01:27:35 -0700 Subject: [PATCH 17/20] Refactor based on the feedback --- source/slang/slang-ir.cpp | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 867fc8220a..524bd9955d 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1691,12 +1691,6 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) { SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); - // Because IRWitnessTable is Hoistable, do not move when - // it is already a child of the parent. - // - if (parent == inst->parent) - return; - // For the rest of the cases, IRWitnessTable goes to the // end of the list but before the terminators. while (insertBeforeInst && !as(insertBeforeInst)) @@ -2635,7 +2629,9 @@ IRInst* IRBuilder::_findOrEmitHoistableInst( } } - addHoistableInst(this, inst); + // When the inst is already a child, skip adding it as Hoistable. + if (inst->parent == nullptr) + addHoistableInst(this, inst); return inst; } @@ -4605,6 +4601,15 @@ IRDominatorTree* IRModule::findOrCreateDominatorTree(IRGlobalValueWithCode* func void addGlobalValue(IRBuilder* builder, IRInst* value) { + // If the value is already in the parent, keep it as-is. + // Because when the inst is Hoistable, the parent can have + // only one instance of the inst. The order among + // siblings should remain because the later siblings may + // have dependency to the earlier siblings. + // + if (value->parent) + return; + // Try to find a suitable parent for the // global value we are emitting. // @@ -4643,18 +4648,6 @@ void addGlobalValue(IRBuilder* builder, IRInst* value) parent = builder->getModule()->getModuleInst(); } - // If the value is already in the parent, keep it as-is. - // Because WitnessTable is Hoistable, the parent can have - // only one instance of this WitnessTable. The order among - // siblings should remain because the later siblings may - // have dependency to the earlier siblings. - // - if (parent == value->parent && value->getOp() == kIROp_WitnessTable) - { - SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); - return; - } - // If it turns out that we are inserting into the // current "insert into" parent for the builder, then // we need to respect its "insert before" setting From 34447fe2b357b94701f16b19e84e49f6c6dc5970 Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Wed, 19 Mar 2025 17:11:20 -0700 Subject: [PATCH 18/20] Adding a new function addDeduplicatedInst that is simpler than addHoistableInst --- source/slang/slang-ir.cpp | 68 +++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 524bd9955d..ebe9f203ae 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1605,11 +1605,7 @@ static IRInst* pickLaterInstInSameParent(IRInst* left, IRInst* right) } } -// Given an instruction that represents a constant, a type, etc. -// Try to "hoist" it as far toward the global scope as possible -// to insert it at a location where it will be maximally visible. -// -void addHoistableInst(IRBuilder* builder, IRInst* inst) +static IRInst* getParentOfHoistableInst(IRBuilder* builder, IRInst* inst) { // Start with the assumption that we would insert this instruction // into the global scope (the instruction that represents the module) @@ -1643,6 +1639,17 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) // SLANG_ASSERT(parent); + return parent; +} + +// Given an instruction that represents a constant, a type, etc. +// Try to "hoist" it as far toward the global scope as possible +// to insert it at a location where it will be maximally visible. +// +void addHoistableInst(IRBuilder* builder, IRInst* inst) +{ + IRInst* parent = getParentOfHoistableInst(builder, inst); + // Once we determine the parent instruction that the // new instruction should be inserted into, we need // to find an appropriate place to insert it. @@ -1687,27 +1694,13 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) // IRInst* insertBeforeInst = parent->getFirstChild(); - if (inst->getOp() == kIROp_WitnessTable) - { - SLANG_ASSERT(getIROpInfo(kIROp_WitnessTable).isHoistable()); - - // For the rest of the cases, IRWitnessTable goes to the - // end of the list but before the terminators. - while (insertBeforeInst && !as(insertBeforeInst)) - { - insertBeforeInst = insertBeforeInst->getNextInst(); - } - } - else + // Hoistable instructions are always "ordinary" + // instructions, so they need to come after + // any parameters of the parent. + // + while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) { - // Hoistable instructions are always "ordinary" - // instructions, so they need to come after - // any parameters of the parent. - // - while (insertBeforeInst && insertBeforeInst->getOp() == kIROp_Param) - { - insertBeforeInst = insertBeforeInst->getNextInst(); - } + insertBeforeInst = insertBeforeInst->getNextInst(); } // For instructions that will be placed at module scope, @@ -1721,6 +1714,7 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) // the operands of `inst` come from the same // block that we insert after them. // + UInt operandCount = inst->getOperandCount(); for (UInt ii = 0; ii < operandCount; ++ii) { auto operand = inst->getOperand(ii); @@ -1756,6 +1750,19 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) } } +// This function finds where a parent should be for the given inst, +// and add the inst as a last child. +// When the inst is marked as Hoistable but the intention is only to de-duplicate it, +// the inst can be added in a simpler manner. +void addDeduplicatedInst(IRBuilder * builder, IRInst * inst) +{ + SLANG_ASSERT(nullptr == inst->parent); + + IRInst* parent = getParentOfHoistableInst(builder, inst); + + inst->insertAtEnd(parent); +} + void IRBuilder::_maybeSetSourceLoc(IRInst* inst) { auto sourceLocInfo = getSourceLocInfo(); @@ -2629,9 +2636,16 @@ IRInst* IRBuilder::_findOrEmitHoistableInst( } } - // When the inst is already a child, skip adding it as Hoistable. + // When an hoistable inst is already a child, skip adding it. if (inst->parent == nullptr) - addHoistableInst(this, inst); + { + // In order to de-duplicate them, Witness-table is marked as Hoistable. + // But it is not exactly a hoistable type and it can be added simpler. + if (inst->getOp() == kIROp_WitnessTable) + addDeduplicatedInst(this, inst); + else + addHoistableInst(this, inst); + } return inst; } From d19ff646fb15aa10648766efd035cc7e2c17234a Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 20 Mar 2025 11:50:44 -0700 Subject: [PATCH 19/20] Simplfy addDeduplicatedInst into addInst --- source/slang/slang-ir.cpp | 46 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index ebe9f203ae..b3af9363dd 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1605,7 +1605,11 @@ static IRInst* pickLaterInstInSameParent(IRInst* left, IRInst* right) } } -static IRInst* getParentOfHoistableInst(IRBuilder* builder, IRInst* inst) +// Given an instruction that represents a constant, a type, etc. +// Try to "hoist" it as far toward the global scope as possible +// to insert it at a location where it will be maximally visible. +// +void addHoistableInst(IRBuilder* builder, IRInst* inst) { // Start with the assumption that we would insert this instruction // into the global scope (the instruction that represents the module) @@ -1639,17 +1643,6 @@ static IRInst* getParentOfHoistableInst(IRBuilder* builder, IRInst* inst) // SLANG_ASSERT(parent); - return parent; -} - -// Given an instruction that represents a constant, a type, etc. -// Try to "hoist" it as far toward the global scope as possible -// to insert it at a location where it will be maximally visible. -// -void addHoistableInst(IRBuilder* builder, IRInst* inst) -{ - IRInst* parent = getParentOfHoistableInst(builder, inst); - // Once we determine the parent instruction that the // new instruction should be inserted into, we need // to find an appropriate place to insert it. @@ -1714,7 +1707,6 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) // the operands of `inst` come from the same // block that we insert after them. // - UInt operandCount = inst->getOperandCount(); for (UInt ii = 0; ii < operandCount; ++ii) { auto operand = inst->getOperand(ii); @@ -1750,15 +1742,29 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) } } -// This function finds where a parent should be for the given inst, -// and add the inst as a last child. -// When the inst is marked as Hoistable but the intention is only to de-duplicate it, -// the inst can be added in a simpler manner. -void addDeduplicatedInst(IRBuilder * builder, IRInst * inst) +// Add the given inst to the parent of its operand. +void addInst(IRInst* inst) { SLANG_ASSERT(nullptr == inst->parent); - IRInst* parent = getParentOfHoistableInst(builder, inst); + IRInst* parent = nullptr; + + UInt operandCount = inst->getOperandCount(); + for (UInt ii = 0; ii < operandCount; ++ii) + { + auto operand = inst->getOperand(ii); + if (!operand) + continue; + + auto operandParent = operand->getParent(); + + parent = mergeCandidateParentsForHoistableInst(parent, operandParent); + } + + if (inst->getFullType()) + { + parent = mergeCandidateParentsForHoistableInst(parent, inst->getFullType()->getParent()); + } inst->insertAtEnd(parent); } @@ -2642,7 +2648,7 @@ IRInst* IRBuilder::_findOrEmitHoistableInst( // In order to de-duplicate them, Witness-table is marked as Hoistable. // But it is not exactly a hoistable type and it can be added simpler. if (inst->getOp() == kIROp_WitnessTable) - addDeduplicatedInst(this, inst); + addInst(inst); else addHoistableInst(this, inst); } From f768ee7fef311bb6f79654635fd0b034d1004cbf Mon Sep 17 00:00:00 2001 From: Jay Kwak <82421531+jkwak-work@users.noreply.github.com> Date: Thu, 20 Mar 2025 18:21:30 -0700 Subject: [PATCH 20/20] Fix funciton name conflict of addInst --- source/slang/slang-ir.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index b3af9363dd..d7c4afedc2 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -1743,7 +1743,7 @@ void addHoistableInst(IRBuilder* builder, IRInst* inst) } // Add the given inst to the parent of its operand. -void addInst(IRInst* inst) +void addDeduplicatedInst(IRInst* inst) { SLANG_ASSERT(nullptr == inst->parent); @@ -2648,7 +2648,7 @@ IRInst* IRBuilder::_findOrEmitHoistableInst( // In order to de-duplicate them, Witness-table is marked as Hoistable. // But it is not exactly a hoistable type and it can be added simpler. if (inst->getOp() == kIROp_WitnessTable) - addInst(inst); + addDeduplicatedInst(inst); else addHoistableInst(this, inst); } @@ -4628,7 +4628,10 @@ void addGlobalValue(IRBuilder* builder, IRInst* value) // have dependency to the earlier siblings. // if (value->parent) + { + SLANG_ASSERT(getIROpInfo(value->getOp()).isHoistable()); return; + } // Try to find a suitable parent for the // global value we are emitting.