-
Notifications
You must be signed in to change notification settings - Fork 837
Intrinsic: binaryen.removable.if.unused #8268
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea... I'll do some experimenting with a nicer API along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(done)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When are we Vacuuming IR that is not yet constructed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be possible to have wasm2js stop doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'm imagining something like this here:
| if (checkDeadIfUnused(target, nullptr)) { | |
| if (target->getFuncAnnotations().deadIfUnused) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check round-tripping of the function-level annotation as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
| @@ -0,0 +1,88 @@ | |||
| ;; RUN: wasm-opt -all --vacuum %s -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we autogenerate the output for this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Regarding the name: do the semantics match any of the existing GCC attributes of |
|
@dschuff Sadly I don't think they match those. gcc's Concretely, this is valid in gcc: x = pure();
foo();
bar(x);
=>
foo();
bar(pure()); // pure function pushed past foo(), which it cannot interact withBut that is not valid if |
|
Maybe |
|
@tlively I think more than removability is possible, though. The full meaning is "has no effects if the value is not used". Perhaps To clarify what I mean by more than removability being possible: imagine some code cannot be removed for technical, type-system or IR structure reasons. It might still be useful to know it has no effects. Not a great example, but: (drop
(block $block (result (ref $A))
(br_if $block
(local.get $value)
(local.get $condition)
)
(call $dead.if.unused)
)
)
(call $foo)The |
|
IIUC, we cannot move it past the |
|
Ah, do you mean because the call might trap or throw? Fair enough. So amend my example to replace |
|
No, we couldn't move a call with this annotation past a |
|
Ok, after discussion offline I agree @tlively 's approach is better here. So I think renaming to This also means that the optimization in this PR is the only thing the intrinsic allows: literally removing it from its current position, in Vacuum, and nothing else. |
|
Updated to the final name |
Based on discussion in
#7574 (comment)
the
@binaryen.removable.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.
edit: updated, original name was
dead.if.unused