-
Notifications
You must be signed in to change notification settings - Fork 859
Account for unaddressed funcs in GlobalEffects #8644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
dc53f95
3cda588
b1af020
4bc9ee9
703dcdd
bafa5f6
7e39398
058a10c
d746b6b
73a7e87
0af5402
c88fe17
2d637dd
83bb32e
d3f907e
7406ec2
8da03f7
0b04bf9
9063fe3
5f89583
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,53 @@ struct FuncInfo { | |
| std::unordered_set<HeapType> indirectCalledTypes; | ||
| }; | ||
|
|
||
| // Only funcs that are 'addressed' may be the target of an indirect call. A | ||
| // function is addressed if: | ||
| // - It appears in a ref.func expression | ||
| // - It appears in an `elem` segment (note that we already ignore `elem declare` | ||
| // statements in our IR, but we check separately for funcs that appear in | ||
| // `ref.func`). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition is subsumed by the the previous condition, so is redundant. Maybe that's what the note is saying, but I think we can just remove this entirely.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the point but included a small note on it still to make it clear how it maps to Wasm (we drop |
||
| // - It's exported, because it may flow back to us as a reference. | ||
| // - It's imported, which implies it can be addressed (see | ||
| // https://github.com/WebAssembly/spec/issues/2072). | ||
|
stevenfontanella marked this conversation as resolved.
Outdated
|
||
| // | ||
| // If a function doesn't meet any of these criteria, it can't be the target of | ||
| // an indirect call and we don't need to include its effects in indirect calls. | ||
| std::unordered_set<Function*> getAddressedFuncs(Module& module) { | ||
| struct AddressedFuncsWalker : WalkerPass<PostWalker<AddressedFuncsWalker>> { | ||
| std::unordered_set<Function*>& addressedFuncs; | ||
|
|
||
| AddressedFuncsWalker(std::unordered_set<Function*>& addressedFuncs) | ||
| : addressedFuncs(addressedFuncs) {} | ||
|
|
||
| bool isFunctionParallel() override { return true; } | ||
|
|
||
| void visitRefFunc(RefFunc* refFunc) { | ||
| addressedFuncs.insert(getModule()->getFunction(refFunc->func)); | ||
| } | ||
| }; | ||
|
|
||
| std::unordered_set<Function*> addressedFuncs; | ||
| AddressedFuncsWalker walker(addressedFuncs); | ||
| walker.walkModuleCode(&module); | ||
| walker.walkModule(&module); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't going to traverse the module in parallel. You need to call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Included it in the existing pass to avoid having to create a ParallelFunctionAnalysis with the same logic. |
||
|
|
||
| ModuleUtils::iterImportedFunctions( | ||
| module, [&addressedFuncs, &module](Function* import) { | ||
| addressedFuncs.insert(module.getFunction(import->name)); | ||
| }); | ||
|
|
||
| for (const auto& export_ : module.exports) { | ||
| if (export_->kind != ExternalKind::Function) { | ||
| continue; | ||
| } | ||
|
|
||
| addressedFuncs.insert(module.getFunction(*export_->getInternalName())); | ||
| } | ||
|
|
||
| return addressedFuncs; | ||
| } | ||
|
|
||
| std::map<Function*, FuncInfo> analyzeFuncs(Module& module, | ||
| const PassOptions& passOptions) { | ||
| ModuleUtils::ParallelFunctionAnalysis<FuncInfo> analysis( | ||
|
|
@@ -144,6 +191,7 @@ using CallGraph = | |
|
|
||
| CallGraph buildCallGraph(const Module& module, | ||
| const std::map<Function*, FuncInfo>& funcInfos, | ||
| const std::unordered_set<Function*>& addressedFuncs, | ||
| WorldMode worldMode) { | ||
| CallGraph callGraph; | ||
| if (worldMode == WorldMode::Open) { | ||
|
|
@@ -179,16 +227,18 @@ CallGraph buildCallGraph(const Module& module, | |
| } | ||
|
|
||
| // Type -> Function | ||
| callGraph[caller->type.getHeapType()].insert(caller); | ||
| if (addressedFuncs.contains(caller)) { | ||
| callGraph[caller->type.getHeapType()].insert(caller); | ||
| } | ||
| } | ||
|
|
||
| // Type -> Type | ||
| // Do a DFS up the type heirarchy for all function implementations. | ||
| // Do a DFS up the type hierarchy for all function implementations. | ||
| // We are essentially walking up each supertype chain and adding edges from | ||
| // super -> subtype, but doing it via DFS to avoid repeated work. | ||
| Graph superTypeGraph(allFunctionTypes.begin(), | ||
| allFunctionTypes.end(), | ||
| [&callGraph](auto&& push, HeapType t) { | ||
| [&callGraph](const auto& push, HeapType t) { | ||
| // Not needed except that during lookup we expect the | ||
| // key to exist. | ||
| callGraph[t]; | ||
|
|
@@ -350,8 +400,10 @@ struct GenerateGlobalEffects : public Pass { | |
| std::map<Function*, FuncInfo> funcInfos = | ||
| analyzeFuncs(*module, getPassOptions()); | ||
|
|
||
| auto callGraph = | ||
| buildCallGraph(*module, funcInfos, getPassOptions().worldMode); | ||
| auto addressedFuncs = getAddressedFuncs(*module); | ||
|
|
||
| auto callGraph = buildCallGraph( | ||
| *module, funcInfos, addressedFuncs, getPassOptions().worldMode); | ||
|
|
||
| propagateEffects(*module, | ||
| getPassOptions(), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.