Conversation
src/wasm.h
Outdated
| // Toolchain hint: If this expression's result is unused, then the entire | ||
| // thing can be considered dead and removable. | ||
| // TODO: link to spec somewhere | ||
| std::optional<std::monostate> deadIfUnused; |
There was a problem hiding this comment.
Is it really better to use a std::optional rather than bool here? It doesn't look like we take advantage of the uniformity of using std::optional in any way.
There was a problem hiding this comment.
I think it's just nice for consistency. Each hint may be present or not, and handling that property uniformly with the same C++ mechanism seems clearer?
There was a problem hiding this comment.
If any of the use sites had to change, I would agree. But I'm pretty sure making this a bool is a strict simplification, even if it is obviously different from the other hints. I think "std::optional if there is associated data and otherwise bool" is still a pretty simple guideline.
src/passes/Vacuum.cpp
Outdated
| auto iter = annotations.find(target); | ||
| if (iter != annotations.end()) { | ||
| auto& annotation = iter->second; |
There was a problem hiding this comment.
It looks like it would be useful to have a func->getAnnotation(curr) helper of some sort to avoid mucking around with map lookups at every callsite. It could even return an empty annotations object when there is no annotation to avoid all branching logic in the caller.
It also seems somewhat magical that querying with a null expression gets the function annotations. I think ideally we would have a separate helper for that.
There was a problem hiding this comment.
Added a nicer helper now.
src/passes/Vacuum.cpp
Outdated
| }; | ||
|
|
||
| // Look for an annotation on the call. | ||
| if (checkDeadIfUnused(getFunction(), call)) { |
There was a problem hiding this comment.
I'm imagining that this could be something like this:
| if (checkDeadIfUnused(getFunction(), call)) { | |
| if (getFunction()->getAnnotations(call).deadIfUnused) { |
There was a problem hiding this comment.
Yeah, that's a good idea... I'll do some experimenting with a nicer API along those lines.
src/passes/Vacuum.cpp
Outdated
| // Check on the called function, if it exists (it may not if the IR is | ||
| // still being built up). |
There was a problem hiding this comment.
When are we Vacuuming IR that is not yet constructed?
There was a problem hiding this comment.
In wasm2js we apparently have situations where we optimize "partial" code, before the module is complete. We used to do that in asm2wasm back in the day (compile one function before later functions were even parsed) but we got rid of asm2wasm.
There was a problem hiding this comment.
Do you think it would be possible to have wasm2js stop doing that?
There was a problem hiding this comment.
It might not be trivial. Looks like the place is processStandaloneFunction which tests use to just handle a function outside of a module. The wasm2js testing stuff is... not simple, unfortunately.
src/passes/Vacuum.cpp
Outdated
| // Check on the called function, if it exists (it may not if the IR is | ||
| // still being built up). | ||
| if (auto* target = getModule()->getFunctionOrNull(call->target)) { | ||
| if (checkDeadIfUnused(target, nullptr)) { |
There was a problem hiding this comment.
And I'm imagining something like this here:
| if (checkDeadIfUnused(target, nullptr)) { | |
| if (target->getFuncAnnotations().deadIfUnused) { |
There was a problem hiding this comment.
Should we check round-tripping of the function-level annotation as well?
| @@ -0,0 +1,88 @@ | |||
| ;; RUN: wasm-opt -all --vacuum %s -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Can we autogenerate the output for this one?
There was a problem hiding this comment.
It looked into this a little but it would be a huge refactoring of the update script. The issue is that module-level things like functions are all tracked by name, but this would not be a named thing, so a very different regex is needed, and different tracking to match it up to the right thing in the output. I did some experimentation, but it got very messy... Though maybe you know that script better and have an idea?
There was a problem hiding this comment.
Oh I see. That's unfortunate. I can take a look at updating the script to handle annotations. We'll definitely want that for e.g. type annotations in the future.
2 participants
Based on discussion in
#7574 (comment)
the
@binaryen.dead.if.unusedcode annotation has the meaning thatif the result is unused (dropped), then the code can be considered
dead (no side effects, removable).
This can be used on a function to affect all calls to it, or on specific
call instructions. The optimizer then finds relevant dropped calls and
can remove them (in Vacuum).
Bikeshedding welcome on the name.