Repository navigation
Conversation
This removes `UsedNames` and directly uses `Module`s in place of that. `UsedNames` was first added in #8221, and after #8986, it becamse largely unnecessary and made code more complicated. Also this removes `owner` field from `ItemInfo`, which can be computed. This also templatize member functions in `OwnershipTracker` so they can be called with a simpler syntax of `tracker.func<T>(name)`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors OwnershipTracker and ModuleSplitter in src/ir/module-splitting.cpp to track module item ownership directly using Module* pointers instead of the intermediate UsedNames struct. It introduces template-based helper methods to handle different module item types uniformly, significantly simplifying the code and removing redundant macros. The review feedback suggests two optimizations: simplifying the conditional logic in getOwner by using an assertion for the single secondary module case, and reducing map lookups in the global sharing loop by introducing a single lookup helper.
| if (tracker.isUsedByPrimary<Global>(global->name)) { | ||
| for (auto* get : gets) { | ||
| tracker.insert<Global>(get->name, &primaryUsed); | ||
| tracker.insert<Global>(get->name, &primary); | ||
| } | ||
| } | ||
| auto& usingSecs = | ||
| tracker.getUsingSecondaries(global->name, tracker.globals); | ||
| for (size_t i = 0; i < secondaries.size(); ++i) { | ||
| Module* sec = secondaries[i].get(); | ||
| if (std::find(usingSecs.begin(), usingSecs.end(), sec) != | ||
| usingSecs.end()) { | ||
| for (auto* get : gets) { | ||
| tracker.insert<Global>(get->name, &secondaryUsed[i]); | ||
| } | ||
| for (Module* sec : tracker.getUsingSecondaries<Global>(global->name)) { | ||
| for (auto* get : gets) { | ||
| tracker.insert<Global>(get->name, sec); | ||
| } | ||
| } |
There was a problem hiding this comment.
We can also optimize this block by using tracker.getItemInfo<Global>(global->name) to perform a single map lookup instead of separate lookups for isUsedByPrimary and getUsingSecondaries.
if (const auto* info = tracker.getItemInfo<Global>(global->name)) {
if (info->usedByPrimary) {
for (auto* get : gets) {
tracker.insert<Global>(get->name, &primary);
}
}
for (Module* sec : info->usingSecondaries) {
for (auto* get : gets) {
tracker.insert<Global>(get->name, sec);
}
}
}There was a problem hiding this comment.
We don't have a method called tracker.getItemInfo. Also creating one doesn't seem to simplify things anyway.
| } else if constexpr (std::is_same_v<T, ElementSegment>) { | ||
| return elementSegments; | ||
| } else { | ||
| static_assert(!sizeof(T*), "Unsupported module item type"); |
There was a problem hiding this comment.
Can this just be static_assert(false, ...)?
There was a problem hiding this comment.
That doesn't work for older compilers: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2593r1.html
We have a similar trick in our codebase too: https://github.com/WebAssembly/binaryen/blame/c35bddec04ca366d8f307ca5fcec98c358239db0/src/tools/fuzzing/random.h#L120-L127
Anyway, changed the code to avoid using the trick: 0785c49
|
Oh and this needs to land after #9064. (Sorry for the long silence there. I addressed the comments there) |
This removes
UsedNamesand directly usesModules in place of that.UsedNameswas first added in #8221, and after #8986, it becamse largely unnecessary and made code more complicated. Also this removesownerfield fromItemInfo, which can be computed.This also templatize member functions in
OwnershipTrackerso they can be called with a simpler syntax oftracker.func<T>(name).