Cranelift: Remove the stack_{load,store} instructions - #13580
#13580Cranelift: Remove the stack_{load,store} instructions#13580fitzgen merged 1 commit intobytecodealliance:mainbytecodealliance/wasmtime:mainfrom fitzgen:wasmtime-remove-stack-load-storefitzgen/wasmtime:wasmtime-remove-stack-load-storeCopy head branch name to clipboard
stack_{load,store} instructions#13580Conversation
alexcrichton
left a comment
There was a problem hiding this comment.
Would it be possible to avoid changing finalize by looking at the stack slot's size field and choosing an appropriate type based on that?
Unfortunately not, because it isn't the stack slot's type that we need to pass in (that was already passed in) it is the size of the target pointer. |
|
So, here, which I belive is the only need for changing |
These get immediately legalized to `stack_addr` and `load` or `store`, so frontends should just emit that pair directly. For convenience and backwards compat, we add `InstBuilder::stack_load` and `InstBuilder::stack_store` methods that emit the pair as a single operation. Unfortunately, these methods have to take an additional pointer-type parameter that the old version didn't have to take because the rewrite didn't happen until legalization time, so these new methods are not 100% backwards compatible. This also necessitated passing in a `TargetFrontendConfig` to `FuncBuilder::finalize`, which resulted in a bit of churn, but is fully mechanical.
03849f6 to
13b161a
Compare
Correct, although I think it is very reasonable to require a target config when building CLIF in general (e.g. a small additional follow up refactor could make all the memcpy functions that want a target config as an argument stop taking that argument and using the same source of truth as finalize).
It does not match. For Wasmtime, for example, the slots are 4 bytes large ( |
alexcrichton
left a comment
There was a problem hiding this comment.
Ah right, indeed. Would it make sense to take in an entire &dyn TargetIsa here instead of just the TargetFrontendConfig? Or alternatively just take the pointer_type instead of TargetFrontendConfig?
Other frontend methods (such as the memcpy stuff mentioned earlier) take All we absolutely need is the pointer type but it feels a bit like overspecializing on the exact use case today, vs what feels "right" / fits with the design of the crate. |
These get immediately legalized to
stack_addrandloadorstore, so frontends should just emit that pair directly. For convenience and backwards compat, we addInstBuilder::stack_loadandInstBuilder::stack_storemethods that emit the pair as a single operation. Unfortunately, these methods have to take an additional pointer-type parameter that the old version didn't have to take because the rewrite didn't happen until legalization time, so these new methods are not 100% backwards compatible. This also necessitated passing in aTargetFrontendConfigtoFuncBuilder::finalize, which resulted in a bit of churn, but is fully mechanical.