Translate Wasm block params into Variables instead of CLIF block params#13711
Translate Wasm block params into Variables instead of CLIF block params#13711fitzgen wants to merge 3 commits into
Variables instead of CLIF block params#13711Conversation
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! This technically looks fine to me, and I agree it's nice to use our existing algorithm to avoid creating blockparams where they are actually trivial/unneeded.
I'm curious, however, since this does make things a little more complex (some blocks take vars, some don't), and leans more on an implicit SSA construction algorithm, which does have some cost, with the hypothesis that we'll get better code / faster compiles with fewer blockparams in the IR: have you measured the improvement? Do we see faster compile or faster runtimes out of this?
| /// For a Wasm control-flow target (i.e. a block with associated parameter | ||
| /// `Variable`s) we `use_var` each of those variables. For a block with real | ||
| /// CLIF block parameters (the function's exit block) we read those parameters | ||
| /// directly. |
There was a problem hiding this comment.
This distinction seems to me like it has potential to be error-prone or at least lead to subtle cases (e.g. there's the implicit invariant that the exit block is not also a Wasm target; we're creating a new bifurcation of kinds-of-blocks). Is there a reason we can't have the exit-block take its return value(s) as variables, too?
There was a problem hiding this comment.
I initially was trying to minimize changes by leaving the return block as-is, but switching it over as well means that basically all blocks are handled uniformly now (using variables instead of block params) except for two cases:
- The entry block, which has function params
- Exception blocks, which take the payload as params
However, both of these kinds of blocks are never targeted for branching directly by e.g. br_if in Wasm so they can never have Wasm stack operand params, so our handling of Wasm branch targets can still be uniform. I think this leaves us in a pretty nice place, but I'm curious what your opinion is too.
See also c20864c
Just did a sightglass run on a subset of the suite recommended by an initial PCA analysis (before going through all the steps I still want to do before the Official analysis) and this PR doesn't have any statistically significant effect on either compilation or execution: Details |
…rams This allows SSA construction to determine whether they need to actually become block params or not, and cuts down on the number of unnecessary block parameters we pessimistically introduce during Wasm-to-CLIF translation.
fitzgen
force-pushed
the
wasmtime-wasm-block-params-should-be-variables
branch
from
71c6d56 to
2b3665c
Compare
2 participants
This allows SSA construction to determine whether they need to actually become block params or not, and cuts down on the number of unnecessary block parameters we pessimistically introduce during Wasm-to-CLIF translation.