Add extension marker to i32 arguments of builtin functions #2354
Conversation
Some platform ABIs require i32 values to be zero- or sign-extended to the full register width. The extension is implemented by the cranelift codegen backend, but this happens only if the appropriate "uext" or "sext" attribute is present in the cranelift IR. For calls to builtin functions, that IR is synthesized by the code in func_environ.rs -- to ensure correct codegen for the target ABI, this code needs to add those attributes as necessary.
|
Hmm -- this will potentially pessimize x64 and aarch64, as on both of those platforms the extension modes are actually used sometimes, and not ignored. Specifically, in the SpiderMonkey embedding (Baldrdash calling convention), the JIT requires all args to be zero-extended, so we do respect the extension modes. This change could thus result in extraneous extension instructions in the non-Baldrdash (i.e. normal SysV convention) case. It's a bit confusing to me why the platform-independent IR leaks platform-specific ABI details, and perhaps we should clarify the design eventually. But in the meantime: is there a reason that the relevant backend(s) (this is an issue with IBM Z, I'm assuming?) can't unconditionally extend args, given that that is the ABI? |
The extension marker specifies that the argument should be extended if and only if required by the ABI. So if there is a difference between ABIs, the back-end should handle this accordingly. The only reason why the extension marker is provided by the front-end is to specify signedness. If the back-end decides the ABI requires extension, then it needs to know whether to zero- or sign-extend, and the only place that information can come from is the front-end. |
|
Interesting -- this doesn't match the definition that I had understood (namely, that the
So the doc-comment spec is somewhat ambiguous, but all three backends currently treat the extension mode as a mandatory request (always extend if specified), so it's certainly the lowest-overhead choice to keep that definition. And, given that we have other Cranelift users out-of-tree, we should consider it a public interface and be hesitant to change it. I think I understand the difficulty here, though: it seems that in the past, the most common use (or at least the initial driving use) of this feature was SpiderMonkey's ABI, for which the environment definition could attach the required extension modes because it controls generation of signatures and callsite IR. No "vanilla" system ABI has had this requirement yet, so for other environments, we have no way to infer the signedness of the value once we get to the backend, as this has to come from the frontend (as you say). That said: at least for the If we truly need a notion of signedness for some ABIs to be implemented properly, then we can definitely discuss how to get this -- perhaps a signedness bit on types. That would need a wider discussion, of course. Anyone else have thoughts on this? |
|
The LLVM LangRef states: https://llvm.org/docs/LangRef.html#parameter-attributes
Unnecessarily extending the arguments shouldn't be observable unless you use a different type on the caller and callee side, which risks causing trouble on some architectures anyway. For this reason I wouldn't consider stopping with unnecessary extension a breaking change.
I don't handle this part of the C abi yet like many other things. I just never add uext/sext.
LLVM doesn't have separate types for unsigned and signed integers either. |
|
Interesting; thanks for the LLVM reference -- a lot of valuable experience to learn from in their design decisions. From first principles, I agree that it should actually make sense to have an "if you need to extend this, here is the signedness" bit of information on parameters; "always extend in this way" makes less sense, because it is hoisting the question of "do we extend", which is an ABI-level question, up to the machine-independent IR. So I'm coming to agree that we should align with that. That said, the two concerns are:
So, to accept this patch, I'd want to do the following things first:
I can help with the latter -- it should be a pretty easy change. As to the former -- @sunfishcode or others, are we aware of any frontends that might break if we no longer unconditionally extend with sext/uext attributes are present? |
|
I went through all mentions of All use only Edit: that doesn't include |
|
In the subset of the crates that grep.app searches, |
Actually, many 64-bit platform ABIs require extension to the full word size. In addition to IBM Z (which is my problem), this is true for Power, Riscv, Mips, Sparc (just at first glance). The only 64-bit platforms that do not require extension to 64 bits are x86_64 and aarch64 as far as I'm aware. (And note that even those still require extension for smaller types to at least 32 bits.)
I guess I could fix my problem for now by always doing an unsigned extend in the backend, if wasmtime types are always unsigned. That just doesn't seem quite right if we want to consider other uses of cranelift going forward (e.g. I understand there's a Rust frontend in progress?). Going back from the general discussion to this specific patch, it is actually not a very far-reaching change in that it only affects calls to certain external builtin functions. (For calls strictly between two wasmtime functions, it does not really matter whether we adhere 100% to the platform ABI, as long as both sides agree. So I haven't really bothered to change all generated calls to do the extension.) Therefore, I'm not sure that any performance impact due to unnecessary extensions on just those builtin calls would actually be measurable on either x86_64 or aarch64. So it may not even be immediately required to follow up with back-end changes there ... |
👋 By the way I noticed that you only applied this change to the |
Ah, I see :-)
As I said in my reply to Chris, I didn't systematically attempt to change all places that generate function call IR, only those where the call interfaces between wasmtime-generated code and some external platform code -- those are the only places where this makes a real difference (without this change, some wasm tests will fail or even crash on my platform). |
|
@bjorn3, thanks for doing that survey, and for creating the issue in the saltwater project! @uweigand: I'll plan to update the extend behavior in the backends, likely tomorrow or Thursday (I actually have a day off today; I should probably disappear soon :-) ). As per my latest message above, I agree that we should align to the "only extend if ABI specifies" behavior. I think it's fine to land this patch once the backends are updated (I'm hesitant to do so beforehand as even small perf regressions can be problematic for some users). Anyway, thanks for the prodding on the extend semantics -- I feel like we're in a much clearer place now! I'll update the doc comment as well so that this is more precisely specified going forward. |
|
@cfallin Thanks! |
You only changed the cranelift-tools crate, which exists for testing purposes only. It isn't used by Wasmtime. You probably also need to change it in Wasmtime to make interfacing between wasm and native code work.
Any change to Wasmtime will not affect other users of Cranelift. Only if you were to change Cranelift to default to a certain extension mode, would it affect other frontends. |
Maybe I'm confused. My understanding is that this patch touches crates/cranelift/src/func_environ.rs, which exports routines to emit calls to builtin functions (using the AbiParam specs I'm modifying), e.g. the translate_memory_grow routine. This is part of a clause Is that not correct? |
I thought it was |
There has been some confusion over the meaning of the "sign-extend" (`sext`) and "zero-extend" (`uext`) attributes on parameters and return values in signatures. According to the three implemented backends, these attributes indicate that a value narrower than a full register should always be extended in the way specified. However, they are much more useful if they mean "extend in this way if the ABI requires extending": only the ABI backend knows whether or not a particular ABI (e.g., x64 SysV vs. x64 Baldrdash) requires extensions, while only the frontend (CLIF generator) knows whether or not a value is signed, so the two have to work in concert. This is the result of some very helpful discussion in bytecodealliance#2354 (thanks to @uweigand for raising the issue and @bjorn3 for helping to reason about it). This change respects the extension attributes in the above way, rather than unconditionally extending, to avoid potential performance degradation as we introduce more extension attributes on signatures.
There has been some confusion over the meaning of the "sign-extend" (`sext`) and "zero-extend" (`uext`) attributes on parameters and return values in signatures. According to the three implemented backends, these attributes indicate that a value narrower than a full register should always be extended in the way specified. However, they are much more useful if they mean "extend in this way if the ABI requires extending": only the ABI backend knows whether or not a particular ABI (e.g., x64 SysV vs. x64 Baldrdash) requires extensions, while only the frontend (CLIF generator) knows whether or not a value is signed, so the two have to work in concert. This is the result of some very helpful discussion in bytecodealliance#2354 (thanks to @uweigand for raising the issue and @bjorn3 for helping to reason about it). This change respects the extension attributes in the above way, rather than unconditionally extending, to avoid potential performance degradation as we introduce more extension attributes on signatures.
There has been some confusion over the meaning of the "sign-extend" (`sext`) and "zero-extend" (`uext`) attributes on parameters and return values in signatures. According to the three implemented backends, these attributes indicate that a value narrower than a full register should always be extended in the way specified. However, they are much more useful if they mean "extend in this way if the ABI requires extending": only the ABI backend knows whether or not a particular ABI (e.g., x64 SysV vs. x64 Baldrdash) requires extensions, while only the frontend (CLIF generator) knows whether or not a value is signed, so the two have to work in concert. This is the result of some very helpful discussion in #2354 (thanks to @uweigand for raising the issue and @bjorn3 for helping to reason about it). This change respects the extension attributes in the above way, rather than unconditionally extending, to avoid potential performance degradation as we introduce more extension attributes on signatures.
Some platform ABIs require i32 values to be zero- or sign-extended
to the full register width. The extension is implemented by the
cranelift codegen backend, but this happens only if the appropriate
"uext" or "sext" attribute is present in the cranelift IR.
For calls to builtin functions, that IR is synthesized by the code
in func_environ.rs -- to ensure correct codegen for the target ABI,
this code needs to add those attributes as necessary.