From c3cb0515f071a7e95e60196a79ffbebf41b7409d Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 25 Nov 2025 09:54:38 -0800 Subject: [PATCH 1/6] go --- src/passes/DeadArgumentElimination.cpp | 70 ++++++++++++++++---------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index 42d8d87a6a9..c8e361f200a 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -233,6 +233,9 @@ struct DAE : public Pass { // and then use that possibly-over-approximating data. std::vector> callers; + // TOCO + Index unprofitableRemovalIters = 0; + bool iteration(Module* module, DAEFunctionInfoMap& infoMap) { allDroppedCalls.clear(); @@ -380,34 +383,47 @@ struct DAE : public Pass { // TODO: We could track in which functions we actually make changes. ReFinalize().run(getPassRunner(), module); } - // We now know which parameters are unused, and can potentially remove them. - for (Index index = 0; index < numFunctions; index++) { - auto* func = module->functions[index].get(); - if (func->imported()) { - continue; - } - if (hasUnseenCalls[index]) { - continue; - } - auto numParams = func->getNumParams(); - if (numParams == 0) { - continue; - } - auto& calls = allCalls[index]; - if (calls.empty()) { - continue; - } - auto name = func->name; - auto [removedIndexes, outcome] = ParamUtils::removeParameters( - {func}, infoMap[name].unusedParams, calls, {}, module, getPassRunner()); - if (!removedIndexes.empty()) { - // Success! - worthOptimizing.insert(func); - markStale(name); - markCallersStale(index); + if (!unprofitableRemovalIters) { + // We now know which parameters are unused, and can potentially remove them. + Index removals = 0; + Index singleCallerRemovals = 0; + for (Index index = 0; index < numFunctions; index++) { + auto* func = module->functions[index].get(); + if (func->imported()) { + continue; + } + if (hasUnseenCalls[index]) { + continue; + } + auto numParams = func->getNumParams(); + if (numParams == 0) { + continue; + } + auto& calls = allCalls[index]; + if (calls.empty()) { + continue; + } + auto name = func->name; + auto [removedIndexes, outcome] = ParamUtils::removeParameters( + {func}, infoMap[name].unusedParams, calls, {}, module, getPassRunner()); + if (!removedIndexes.empty()) { + // Success! + worthOptimizing.insert(func); + markStale(name); + markCallersStale(index); + if (callers[index].size() == 1) { + singleCallerRemovals++; + } + removals++; + } + if (outcome == ParamUtils::RemovalOutcome::Failure) { + callTargetsToLocalize.insert(name); + } } - if (outcome == ParamUtils::RemovalOutcome::Failure) { - callTargetsToLocalize.insert(name); + if (removals == 1 && singleCallerRemovals == 1 && callTargetsToLocalize.empty()) { + // We only removed parameters from one function, and it had a single + // caller, which is something the inlining pass will handle anyhow. + unprofitableRemovalIters++; } } // We can also tell which calls have all their return values dropped. Note From dbfc5221d1a0a52607bebfb8b30b57e102180abe Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 25 Nov 2025 16:27:53 -0800 Subject: [PATCH 2/6] comment --- src/passes/DeadArgumentElimination.cpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index c8e361f200a..c178242eb97 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -233,7 +233,20 @@ struct DAE : public Pass { // and then use that possibly-over-approximating data. std::vector> callers; - // TOCO + // A count of how many iterations we saw unprofitable removals of parameters. + // An unprofitable removal is one where we only manage to remove from a single + // call, that is, from one call target and it has a single call going to it. + // Such calls are not very interesting, as when there is a single call like + // that then inlining will handle it anyhow, in most cases, and inlining + // does so far more efficiently in situations of call chains: + // a -> b -> c -> d + // + // Imagine we remove a param from d, and so we remove it from the call in c. + // If c received that as a parameter, and only ever used it to call d, then + // now we can remove a param from c, and from the call in b, and so forth - + // requiring a full iteration each time to find the small amount of progress. + // (Inlining, otoh, will inline b into a, then c into a, and d into a, + // efficiently.) Index unprofitableRemovalIters = 0; bool iteration(Module* module, DAEFunctionInfoMap& infoMap) { @@ -383,8 +396,13 @@ struct DAE : public Pass { // TODO: We could track in which functions we actually make changes. ReFinalize().run(getPassRunner(), module); } + // We now know which parameters are unused, and can potentially remove them. + // Only do so if we didn't run into unprofitable removals - if so, leave + // any further removals for other invocations of this pass. (This avoids us + // getting stuck in long unprofitable call chains as mentioned in the + // comment earlier; note that we do process one unprofitable iteration + // before giving up here, so we do make progress at least.) if (!unprofitableRemovalIters) { - // We now know which parameters are unused, and can potentially remove them. Index removals = 0; Index singleCallerRemovals = 0; for (Index index = 0; index < numFunctions; index++) { From 3454e0fe5d60f2ff6e62f738653712a850e3587e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 25 Nov 2025 16:28:37 -0800 Subject: [PATCH 3/6] comment --- src/passes/DeadArgumentElimination.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index c178242eb97..63d952d2fa4 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -440,7 +440,8 @@ struct DAE : public Pass { } if (removals == 1 && singleCallerRemovals == 1 && callTargetsToLocalize.empty()) { // We only removed parameters from one function, and it had a single - // caller, which is something the inlining pass will handle anyhow. + // caller, and we don't have other pending actions (call targets we + // need to localize), so this was unprofitable as mentioned earlier. unprofitableRemovalIters++; } } From 1839982a84b2ebd20891cd0c7968925433c5b649 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 25 Nov 2025 16:28:47 -0800 Subject: [PATCH 4/6] format --- src/passes/DeadArgumentElimination.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index 63d952d2fa4..f7e7816caae 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -422,8 +422,13 @@ struct DAE : public Pass { continue; } auto name = func->name; - auto [removedIndexes, outcome] = ParamUtils::removeParameters( - {func}, infoMap[name].unusedParams, calls, {}, module, getPassRunner()); + auto [removedIndexes, outcome] = + ParamUtils::removeParameters({func}, + infoMap[name].unusedParams, + calls, + {}, + module, + getPassRunner()); if (!removedIndexes.empty()) { // Success! worthOptimizing.insert(func); @@ -438,7 +443,8 @@ struct DAE : public Pass { callTargetsToLocalize.insert(name); } } - if (removals == 1 && singleCallerRemovals == 1 && callTargetsToLocalize.empty()) { + if (removals == 1 && singleCallerRemovals == 1 && + callTargetsToLocalize.empty()) { // We only removed parameters from one function, and it had a single // caller, and we don't have other pending actions (call targets we // need to localize), so this was unprofitable as mentioned earlier. From 8912da21bf836703088d493ea50879797594e066 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 26 Nov 2025 11:50:41 -0800 Subject: [PATCH 5/6] fix --- src/passes/DeadArgumentElimination.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index f7e7816caae..92fe3cc15d4 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -434,7 +434,7 @@ struct DAE : public Pass { worthOptimizing.insert(func); markStale(name); markCallersStale(index); - if (callers[index].size() == 1) { + if (calls.size() == 1) { singleCallerRemovals++; } removals++; From bca19a5a7323bcb2abb7ce2fd517b5a49abd36aa Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 26 Nov 2025 12:09:32 -0800 Subject: [PATCH 6/6] comment --- src/passes/DeadArgumentElimination.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp index 92fe3cc15d4..27cc915e0a4 100644 --- a/src/passes/DeadArgumentElimination.cpp +++ b/src/passes/DeadArgumentElimination.cpp @@ -239,6 +239,7 @@ struct DAE : public Pass { // Such calls are not very interesting, as when there is a single call like // that then inlining will handle it anyhow, in most cases, and inlining // does so far more efficiently in situations of call chains: + // // a -> b -> c -> d // // Imagine we remove a param from d, and so we remove it from the call in c.