Monomorphization: Optimize constants #6711
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Comments on the code, will look at tests in the morning.
| } | ||
|
|
||
| std::unique_ptr<Function> | ||
| copyFunctionWithoutAdd(Function* func, |
There was a problem hiding this comment.
Maybe it would be better to rename copyFunction to copyAndAddFunction as a preliminary change so this can just be copyFunction? Alternatively, we could keep only the behavior of copying without adding; it's not too burdensome for callers to call module.addFunction themselves.
There was a problem hiding this comment.
A downside to copyAndAddFunction (or to keeping the name but dropping the add) is that then it would not be consistent with copyGlobal, copyTable, etc. which all do the natural thing and add.
It is the not-adding that is the slightly odd thing here, but this code really benefits from it, and it seems not that big a burden to have a public API for it I think.
There was a problem hiding this comment.
We definitely want consistency. IMO it would be cleanest if none of the copyXXX functions did the add because then they would all be pure, but no need to block this PR on that.
| // The empirical approach significantly reduces the need for heuristics. For | ||
| // example, rather than have a heuristic for "see if a constant parameter flows | ||
| // into a conditional branch," we simply run the optimizer and let it optimize | ||
| // that case. All other cases handled by the optimizer work as well, without | ||
| // needing to specify them as heuristics, so this gets smarter as the optimizer | ||
| // does. |
There was a problem hiding this comment.
What is the performance overhead of this approach? It would be interesting to know what percent of the work turns out to be not beneficial and thrown away.
There was a problem hiding this comment.
Most of the work is thrown out, I'd bet, but how much depends on heuristics not yet implemented, like we could ignore functions over a certain size, etc.
Overall I think it is fine to waste work here: that's ok if it lets us find optimization opportunities not possible any other way (which I hope is the case 😄 but have not yet proven, though I do have particular use cases that I am confident about).
Fundamentally, rigid heuristics may save work but miss opportunities, while try-it-and-see can find more in return for throwing some work away.
src/passes/Monomorphize.cpp
Outdated
| } | ||
| } | ||
|
|
||
| return dropped == other.dropped; |
There was a problem hiding this comment.
Maybe check this before the operands because it is so cheap?
| bool canBeMovedIntoContext(Expression* curr) { | ||
| // Constant numbers, funcs, strings, etc. can all be copied, so it is ok to | ||
| // add them to the context. | ||
| return Properties::isSingleConstantExpression(curr); |
There was a problem hiding this comment.
It looks like this is not true for constant global.get, but that might be good to include as well.
There was a problem hiding this comment.
Good point, yeah, this could capture anything "copyable" really. I added a TODO.
src/passes/Monomorphize.cpp
Outdated
| [[maybe_unused]] std::ostream& operator<<(std::ostream& o, | ||
| wasm::CallContext& context) { |
There was a problem hiding this comment.
If you name this dump() or something, it will be callable from a debugger, and therefore even more useful.
There was a problem hiding this comment.
I never use a debugger myself, so I would never have thought of that 😄 Done.
src/passes/Monomorphize.cpp
Outdated
| // TODO: check for either a size decrease (always good) or a significant | ||
| // speed increase (as a tiny one, in a huge function, can lead to | ||
| // wasteful duplicated code) |
There was a problem hiding this comment.
Why is code size decreasing always good? Do we add the monomorphized function only when we can entirely replace the original function?
There was a problem hiding this comment.
Good catch, yeah, this TODO makes more sense for the case we can entirely replace, which is not the case here. I revised it.
src/passes/Monomorphize.cpp
Outdated
| } | ||
|
|
||
| // The main body of the function is simply copied from the original. | ||
| auto* newBody = ExpressionManipulator::copy(func->body, wasm); |
There was a problem hiding this comment.
Haven't we already made a copy of the body when we copied the function? Why do we need another copy?
There was a problem hiding this comment.
Good catch, this was a leftover from an older approach. Fixed.
2 participants
Previously the pass would monomorphize a call when we were sending more
refined types than the target expects. This generalizes the pass to also consider
the case where we send a constant in a parameter.
To achieve that, this refactors the pass to explicitly define the "call context",
which is the code around the call (inputs and outputs) that may end up leading
to optimization opportunities when combined with the target function. Also
add comments about the overall design + roadmap.
The existing test is mostly unmodified, and the diff there is smaller when
ignoring whitespace. We do "regress" those tests by adding more local.set
operations, as in the refactoring that makes things a lot simpler, that is, to
handle the general case of an operand having either a refined type or be a
constant, we copy it inside the function, which works either way. This
"regression" is only in the testing version of the pass (the normal version
runs optimizations, which would remove that extra code).
This also enables the pass when GC is disabled. Previously we only handled
refined types, so only GC could benefit. Add a test for MVP content
specifically to show we operate there as well.