https://github.com/hekota created https://github.com/llvm/llvm-project/pull/221662
This change rewrites the `SPIRVLegalizeImplicitBinding` pass so that binding collection is driven by intrinsic declarations rather than a full-module instruction walk. Previously, the pass used an `InstVisitor` to visit every instruction in the module and find calls to a small set of intrinsics. The new implementation iterates over the intrinsic declarations and their users, making the cost proportional to the number of relevant resource calls rather than the size of the module. There is no change to the SPIR-V emitted for existing shaders. Implicit bindings are still assigned the same numbers. Other improvements include: - Order IDs are extracted once into a `SmallVector` instead of being recomputed on each sort comparisons. - The separate `verifyUniqueOrderIdPerResource` walk is folded into the binding assignment loop. - The two near-identical `replaceResourceHandleCall` / `replaceCounterHandleCall` helpers are merged shared code that differs only in intrinsic ID and argument list. - Added a `HasImplicitBinding` flag for early exit and accurate `Changed` reporting, so a module with no implicit bindings (or with a declaration that has no users) now returns `PreservedAnalyses::all()` instead of unconditionally invalidating analyses. >From 8268a49f564c746141d92f0ff4b7c8d878b6dea3 Mon Sep 17 00:00:00 2001 From: Helena Kotas <[email protected]> Date: Sun, 6 Sep 2026 23:57:49 -0700 Subject: [PATCH] [SPIRV] Refactor implicit binding legalization This change rewrites the `SPIRVLegalizeImplicitBinding` pass so that binding collection is driven by intrinsic declarations rather than a full-module instruction walk. Previously, the pass used an `InstVisitor` to visit every instruction in the module and find calls to a small set of intrinsics. The new implementation iterates over the intrinsic declarations and their users, making the cost proportional to the number of relevant resource calls rather than the size of the module. There is no change to the SPIR-V emitted for existing shaders. Implicit bindings are still assigned the same numbers. Other improvements include: - Order IDs are extracted once into a `SmallVector` instead of being recomputed on each sort comparisons. - The separate `verifyUniqueOrderIdPerResource` walk is folded into the binding assignment loop. - The two near-identical `replaceResourceHandleCall` / `replaceCounterHandleCall` helpers are merged shared code that differs only in intrinsic ID and argument list. - Added a `HasImplicitBinding` flag for early exit and accurate `Changed` reporting, so a module with no implicit bindings (or with a declaration that has no users) now returns `PreservedAnalyses::all()` instead of unconditionally invalidating analyses. --- .../SPIRV/SPIRVLegalizeImplicitBinding.cpp | 288 ++++++++---------- 1 file changed, 129 insertions(+), 159 deletions(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVLegalizeImplicitBinding.cpp b/llvm/lib/Target/SPIRV/SPIRVLegalizeImplicitBinding.cpp index 738ba182bd273..bfdcc1370116e 100644 --- a/llvm/lib/Target/SPIRV/SPIRVLegalizeImplicitBinding.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVLegalizeImplicitBinding.cpp @@ -35,17 +35,14 @@ class SPIRVLegalizeImplicitBindingImpl { private: void collectBindingInfo(Module &M); uint32_t getAndReserveFirstUnusedBinding(uint32_t DescSet); - void replaceImplicitBindingCalls(Module &M); - void replaceResourceHandleCall(Module &M, CallInst *OldCI, - uint32_t NewBinding); - void replaceCounterHandleCall(Module &M, CallInst *OldCI, - uint32_t NewBinding); - void verifyUniqueOrderIdPerResource(SmallVectorImpl<CallInst *> &Calls); + bool replaceImplicitBindingCalls(Module &M); // A map from descriptor set to a bit vector of used binding numbers. std::vector<BitVector> UsedBindings; - // A list of all implicit binding calls, to be sorted by order ID. - SmallVector<CallInst *, 16> ImplicitBindingCalls; + + // Set to true by collectBindingInfo() if there are any implicit binding + // calls in the module. + bool HasImplicitBinding = false; }; class SPIRVLegalizeImplicitBindingLegacy : public ModulePass { @@ -60,73 +57,10 @@ class SPIRVLegalizeImplicitBindingLegacy : public ModulePass { } }; -struct BindingInfoCollector : public InstVisitor<BindingInfoCollector> { - std::vector<BitVector> &UsedBindings; - SmallVector<CallInst *, 16> &ImplicitBindingCalls; - - BindingInfoCollector(std::vector<BitVector> &UsedBindings, - SmallVector<CallInst *, 16> &ImplicitBindingCalls) - : UsedBindings(UsedBindings), ImplicitBindingCalls(ImplicitBindingCalls) { - } - - void addBinding(uint32_t DescSet, uint32_t Binding) { - if (UsedBindings.size() <= DescSet) { - UsedBindings.resize(DescSet + 1); - UsedBindings[DescSet].resize(64); - } - if (UsedBindings[DescSet].size() <= Binding) { - UsedBindings[DescSet].resize(2 * Binding + 1); - } - UsedBindings[DescSet].set(Binding); - } - - void visitCallInst(CallInst &CI) { - if (CI.getIntrinsicID() == Intrinsic::spv_resource_handlefrombinding) { - const uint32_t DescSet = - cast<ConstantInt>(CI.getArgOperand(0))->getZExtValue(); - const uint32_t Binding = - cast<ConstantInt>(CI.getArgOperand(1))->getZExtValue(); - addBinding(DescSet, Binding); - } else if (CI.getIntrinsicID() == - Intrinsic::spv_resource_handlefromimplicitbinding) { - ImplicitBindingCalls.push_back(&CI); - } else if (CI.getIntrinsicID() == - Intrinsic::spv_resource_counterhandlefrombinding) { - const uint32_t DescSet = - cast<ConstantInt>(CI.getArgOperand(1))->getZExtValue(); - const uint32_t Binding = - cast<ConstantInt>(CI.getArgOperand(2))->getZExtValue(); - addBinding(DescSet, Binding); - } else if (CI.getIntrinsicID() == - Intrinsic::spv_resource_counterhandlefromimplicitbinding) { - ImplicitBindingCalls.push_back(&CI); - } - } -}; - -static uint32_t getOrderId(const CallInst *CI) { - uint32_t OrderIdArgIdx = 0; - switch (CI->getIntrinsicID()) { - case Intrinsic::spv_resource_handlefromimplicitbinding: - OrderIdArgIdx = 0; - break; - case Intrinsic::spv_resource_counterhandlefromimplicitbinding: - OrderIdArgIdx = 1; - break; - default: - llvm_unreachable("CallInst is not an implicit binding intrinsic"); - } - return cast<ConstantInt>(CI->getArgOperand(OrderIdArgIdx))->getZExtValue(); -} - static uint32_t getDescSet(const CallInst *CI) { uint32_t DescSetArgIdx; switch (CI->getIntrinsicID()) { - case Intrinsic::spv_resource_handlefrombinding: - DescSetArgIdx = 0; - break; case Intrinsic::spv_resource_handlefromimplicitbinding: - case Intrinsic::spv_resource_counterhandlefrombinding: DescSetArgIdx = 1; break; case Intrinsic::spv_resource_counterhandlefromimplicitbinding: @@ -138,30 +72,53 @@ static uint32_t getDescSet(const CallInst *CI) { return cast<ConstantInt>(CI->getArgOperand(DescSetArgIdx))->getZExtValue(); } +// Collect all of the bindings used by llvm.spv.resource.handlefrombinding +// and llvm.spv.resource.counterhandlefrombinding calls. Also check if there +// are any implicit binding calls. void SPIRVLegalizeImplicitBindingImpl::collectBindingInfo(Module &M) { - BindingInfoCollector InfoCollector(UsedBindings, ImplicitBindingCalls); - InfoCollector.visit(M); - - // Sort the collected calls by their order ID. - llvm::sort(ImplicitBindingCalls, [](const CallInst *A, const CallInst *B) { - return getOrderId(A) < getOrderId(B); - }); -} -void SPIRVLegalizeImplicitBindingImpl::verifyUniqueOrderIdPerResource( - SmallVectorImpl<CallInst *> &Calls) { - // Check that the order Id is unique per resource. - for (uint32_t i = 1; i < Calls.size(); ++i) { - const uint32_t OrderA = getOrderId(Calls[i - 1]); - const uint32_t OrderB = getOrderId(Calls[i]); - if (OrderA == OrderB) { - const uint32_t DescSetA = getDescSet(Calls[i - 1]); - const uint32_t DescSetB = getDescSet(Calls[i]); - if (DescSetA != DescSetB) { - report_fatal_error("Implicit binding calls with the same order ID must " - "have the same descriptor set"); + auto addBinding = [&](uint32_t DescSet, uint32_t Binding) { + if (UsedBindings.size() <= DescSet) { + UsedBindings.resize(DescSet + 1); + UsedBindings[DescSet].resize(64); + } + if (UsedBindings[DescSet].size() <= Binding) { + UsedBindings[DescSet].resize(2 * Binding + 1); + } + UsedBindings[DescSet].set(Binding); + }; + + auto collectBinding = [&](Function &F, uint32_t ArgDescSetIdx, + uint32_t ArgBindingIdx) { + for (User *U : F.users()) { + if (CallInst *CI = dyn_cast<CallInst>(U)) { + const uint32_t DescSet = + cast<ConstantInt>(CI->getArgOperand(ArgDescSetIdx))->getZExtValue(); + const uint32_t Binding = + cast<ConstantInt>(CI->getArgOperand(ArgBindingIdx))->getZExtValue(); + addBinding(DescSet, Binding); } } + }; + + for (Function &F : M.functions()) { + if (!F.isDeclaration()) + continue; + + switch (F.getIntrinsicID()) { + case Intrinsic::spv_resource_handlefrombinding: + collectBinding(F, 0, 1); + break; + case Intrinsic::spv_resource_counterhandlefrombinding: + collectBinding(F, 1, 2); + break; + case Intrinsic::spv_resource_handlefromimplicitbinding: + case Intrinsic::spv_resource_counterhandlefromimplicitbinding: + HasImplicitBinding = true; + break; + default: + break; + } } } @@ -182,43 +139,101 @@ uint32_t SPIRVLegalizeImplicitBindingImpl::getAndReserveFirstUnusedBinding( return NewBinding; } -void SPIRVLegalizeImplicitBindingImpl::replaceImplicitBindingCalls(Module &M) { - uint32_t lastOrderId = -1; - uint32_t lastBindingNumber = -1; +bool SPIRVLegalizeImplicitBindingImpl::replaceImplicitBindingCalls(Module &M) { + // Collect all implicit binding calls. + SmallVector<std::pair<uint32_t, CallInst *>, 8> IBCalls; + bool Changed = false; + for (Function &F : M) { + if (!F.isDeclaration()) + continue; + + uint32_t OrderIdIdx; + if (F.getIntrinsicID() == Intrinsic::spv_resource_handlefromimplicitbinding) + OrderIdIdx = 0; + else if (F.getIntrinsicID() == + Intrinsic::spv_resource_counterhandlefromimplicitbinding) + OrderIdIdx = 1; + else + continue; + + for (User *U : F.users()) { + if (CallInst *CI = dyn_cast<CallInst>(U)) { + ConstantInt *OrderId = cast<ConstantInt>(CI->getArgOperand(OrderIdIdx)); + IBCalls.emplace_back(OrderId->getZExtValue(), CI); + } + } + } + + // Sort the collected calls by their order ID. + llvm::sort(IBCalls, [](const std::pair<uint32_t, CallInst *> &A, + const std::pair<uint32_t, CallInst *> &B) { + return A.first < B.first; + }); - for (CallInst *OldCI : ImplicitBindingCalls) { - const uint32_t OrderId = getOrderId(OldCI); - uint32_t BindingNumber; - if (OrderId == lastOrderId) { - BindingNumber = lastBindingNumber; + // Assign bindings based on the order ID. Same order ID gets the same binding. + // Also make sure that calls with the same order ID have the same descriptor + // set. + uint32_t LastOrderId = -1; + uint32_t LastBinding = -1; + uint32_t LastDescSet = -1; + for (auto [OrderId, CI] : IBCalls) { + uint32_t Binding; + uint32_t DescSet = getDescSet(CI); + if (OrderId == LastOrderId) { + if (LastDescSet != DescSet) + report_fatal_error("Implicit binding calls with the same order ID must " + "have the same descriptor set"); + Binding = LastBinding; } else { - const uint32_t DescSet = getDescSet(OldCI); - BindingNumber = getAndReserveFirstUnusedBinding(DescSet); + Binding = getAndReserveFirstUnusedBinding(DescSet); } - if (OldCI->getIntrinsicID() == + // Replace the implicit binding call with a new call using the new binding. + IRBuilder<> Builder(CI); + Intrinsic::ID IID; + SmallVector<Value *, 5> Args; + SmallVector<Type *, 2> OverloadTys = {CI->getType()}; + if (CI->getIntrinsicID() == Intrinsic::spv_resource_handlefromimplicitbinding) { - replaceResourceHandleCall(M, OldCI, BindingNumber); + IID = Intrinsic::spv_resource_handlefrombinding; + Args.push_back(Builder.getInt32(DescSet)); + Args.push_back(Builder.getInt32(Binding)); + // Copy the remaining arguments from the old call. + for (uint32_t i = 2; i < CI->arg_size(); ++i) + Args.push_back(CI->getArgOperand(i)); } else { - assert(OldCI->getIntrinsicID() == + assert(CI->getIntrinsicID() == Intrinsic::spv_resource_counterhandlefromimplicitbinding && - "Unexpected implicit binding intrinsic"); - replaceCounterHandleCall(M, OldCI, BindingNumber); + "unexpected implicit binding intrinsic"); + IID = Intrinsic::spv_resource_counterhandlefrombinding; + Args.push_back(CI->getArgOperand(0)); + Args.push_back(Builder.getInt32(DescSet)); + Args.push_back(Builder.getInt32(Binding)); + OverloadTys.push_back(CI->getArgOperand(0)->getType()); } - lastOrderId = OrderId; - lastBindingNumber = BindingNumber; + Function *NewFunc = Intrinsic::getOrInsertDeclaration(&M, IID, OverloadTys); + CallInst *NewCI = Builder.CreateCall(NewFunc, Args); + NewCI->setCallingConv(CI->getCallingConv()); + + CI->replaceAllUsesWith(NewCI); + CI->eraseFromParent(); + Changed = true; + + LastOrderId = OrderId; + LastBinding = Binding; + LastDescSet = DescSet; } + return Changed; } bool SPIRVLegalizeImplicitBindingImpl::runOnModule(Module &M) { collectBindingInfo(M); - if (ImplicitBindingCalls.empty()) { - return false; - } - verifyUniqueOrderIdPerResource(ImplicitBindingCalls); - replaceImplicitBindingCalls(M); - return true; + bool Changed = false; + if (HasImplicitBinding) + Changed |= replaceImplicitBindingCalls(M); + + return Changed; } } // namespace @@ -238,48 +253,3 @@ INITIALIZE_PASS(SPIRVLegalizeImplicitBindingLegacy, ModulePass *llvm::createSPIRVLegalizeImplicitBindingPass() { return new SPIRVLegalizeImplicitBindingLegacy(); } - -void SPIRVLegalizeImplicitBindingImpl::replaceResourceHandleCall( - Module &M, CallInst *OldCI, uint32_t NewBinding) { - IRBuilder<> Builder(OldCI); - const uint32_t DescSet = - cast<ConstantInt>(OldCI->getArgOperand(1))->getZExtValue(); - - SmallVector<Value *, 8> Args; - Args.push_back(Builder.getInt32(DescSet)); - Args.push_back(Builder.getInt32(NewBinding)); - - // Copy the remaining arguments from the old call. - for (uint32_t i = 2; i < OldCI->arg_size(); ++i) { - Args.push_back(OldCI->getArgOperand(i)); - } - - Function *NewFunc = Intrinsic::getOrInsertDeclaration( - &M, Intrinsic::spv_resource_handlefrombinding, OldCI->getType()); - CallInst *NewCI = Builder.CreateCall(NewFunc, Args); - NewCI->setCallingConv(OldCI->getCallingConv()); - - OldCI->replaceAllUsesWith(NewCI); - OldCI->eraseFromParent(); -} - -void SPIRVLegalizeImplicitBindingImpl::replaceCounterHandleCall( - Module &M, CallInst *OldCI, uint32_t NewBinding) { - IRBuilder<> Builder(OldCI); - const uint32_t DescSet = - cast<ConstantInt>(OldCI->getArgOperand(2))->getZExtValue(); - - SmallVector<Value *, 8> Args; - Args.push_back(OldCI->getArgOperand(0)); - Args.push_back(Builder.getInt32(DescSet)); - Args.push_back(Builder.getInt32(NewBinding)); - - Type *Tys[] = {OldCI->getType(), OldCI->getArgOperand(0)->getType()}; - Function *NewFunc = Intrinsic::getOrInsertDeclaration( - &M, Intrinsic::spv_resource_counterhandlefrombinding, Tys); - CallInst *NewCI = Builder.CreateCall(NewFunc, Args); - NewCI->setCallingConv(OldCI->getCallingConv()); - - OldCI->replaceAllUsesWith(NewCI); - OldCI->eraseFromParent(); -} _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
