Wasmtime GitHub notifications bot (Nov 03 2020 at 17:12): Wasmtime GitHub notifications bot (Nov 03 2020 at 17:12):cfallin commented on Issue #2354:
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?
Wasmtime GitHub notifications bot (Nov 03 2020 at 17:21):uweigand commented on Issue #2354:
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.
Wasmtime GitHub notifications bot (Nov 03 2020 at 17:52):cfallin commented on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtensionif present indicates that the argument must be extended as specified), so I went digging a bit:
In the doc-comment for
ArgumentExtension, all that is specified is "on some architectures, small integer function arguments are extended" (link) -- somewhat ambiguous, so let's look at what backends do...In the current (production) x86 backend, it seems that
uextandsextwill unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtensionunconditionally results in extend ops (fromABIMachineSpec::gen_extend()) ifUextorSext: linkSo 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 this requirement, 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
cranelift-wasmcrate and Wasmtime, we don't have a signed-I32type so I'm curious where the signedness may come from. In this patch, we're just unconditionally adding auextattribute. Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes?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?
Wasmtime GitHub notifications bot (Nov 03 2020 at 17:54):cfallin edited a comment on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtensionif present indicates that the argument must be extended as specified), so I went digging a bit:
In the doc-comment for
ArgumentExtension, all that is specified is "on some architectures, small integer function arguments are extended" (link) -- somewhat ambiguous, so let's look at what backends do...In the current (production) x86 backend, it seems that
uextandsextwill unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtensionunconditionally results in extend ops (fromABIMachineSpec::gen_extend()) ifUextorSext: linkSo 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
cranelift-wasmcrate and Wasmtime, we don't have a signed-I32type so I'm curious where the signedness may come from. In this patch, we're just unconditionally adding auextattribute. Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes?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?
Wasmtime GitHub notifications bot (Nov 03 2020 at 17:56):cfallin edited a comment on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtensionif present indicates that the argument must be extended as specified), so I went digging a bit:
In the doc-comment for
ArgumentExtension, all that is specified is "on some architectures, small integer function arguments are extended" (link) -- somewhat ambiguous, so let's look at what backends do...In the current (production) x86 backend, it seems that
uextandsextwill unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtensionunconditionally results in extend ops (fromABIMachineSpec::gen_extend()) ifUextorSext: linkSo 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
cranelift-wasmcrate and Wasmtime, we don't have a signed-I32type so I'm curious where the signedness may come from. In this patch, we're just unconditionally adding auextattribute. Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes? (Edit: @bjorn3, how doescg_clifhandle signedness -- does it add uext/sext attributes?)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?
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:02):bjorn3 commented on Issue #2354:
The LLVM LangRef states:
https://llvm.org/docs/LangRef.html#parameter-attributes
<dl>
<dt>zeroext</dt>
<dd>This indicates to the code generator that the parameter or return value should be zero-extended to the extent required by the target’s ABI by the caller (for a parameter) or the callee (for a return value).</dd>
<dt>signext</dt>
<dd>This indicates to the code generator that the parameter or return value should be sign-extended to the extent required by the target’s ABI (which is usually 32-bits) by the caller (for a parameter) or the callee (for a return value).</dd>
</dl>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.
Edit: @bjorn3, how does cg_clif handle signedness -- does it add uext/sext attributes?
I don't handle this part of the C abi yet like many other things. I just never add uext/sext.
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?
LLVM doesn't have separate types for unsigned and signed integers either.
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:20):cfallin commented on Issue #2354:
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:
- Performance: in the current backends, we always do the extends if extend-mode is specified. So we would want to alter this logic to "only extend if ABI requires it". But...
- Compatibility: this is a breaking change in the sense that a user could currently be using the default ABI (e.g.
SystemV), yet specifying extend-modes, and relying on the behavior that we always extend. The only case I am currently aware of where extensions matter is in SpiderMonkey, where we specify a different ABI anyway, so that's fine; but perhaps there are other uses for which this is not true.So, to accept this patch, I'd want to do the following things first:
- Verify that we don't have other users (frontends that generate CLIF directly, rather than through the wasm crate) that would rely on the current always-extend behavior.
- Alter the behavior in the three backends (current-x86, new-x64, aarch64) to extend only in the
BaldrdashABI, so we don't have performance regressions.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?
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:46):bjorn3 commented on Issue #2354:
I went through all mentions of
ArgumentExtensionon github. After ignoring all vendorings of cranelift, I found the following uses:https://github.com/nebulet/nebulet/blob/f171d5ee81306e536f60af3617a12d82c5918e69/src/wasm/mod.rs
All use only
ArgumentExtension::None.
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:48):bjorn3 edited a comment on Issue #2354:
I went through all mentions of
ArgumentExtensionon github. After ignoring all vendorings of cranelift, I found the following uses:https://github.com/nebulet/nebulet/blob/f171d5ee81306e536f60af3617a12d82c5918e69/src/wasm/mod.rs
All use only
ArgumentExtension::None.Edit: that doesn't include
.uext()and.sext().
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:49):bjorn3 commented on Issue #2354:
In the subset of the crates that grep.app searches,
.uext()and.sext()are only used inside a test of Cranelift.
Wasmtime GitHub notifications bot (Nov 03 2020 at 18:53):uweigand commented on Issue #2354:
The only case I am currently aware of where extensions matter is in SpiderMonkey, where we specify a different ABI anyway, so that's fine; but perhaps there are other uses for which this is not true.
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.)
That said: at least for the cranelift-wasm crate and Wasmtime, we don't have a signed-I32 type so I'm curious where the signedness may come from. In this patch, we're just unconditionally adding a uext attribute. Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes?
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 ...
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:02):bjorn3 commented on Issue #2354:
(e.g. I understand there's a Rust frontend in progress?).
:wave:
By the way I noticed that you only applied this change to the
cranelift-toolscrate, not Wasmtime or Cranelift itself.
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:12):uweigand commented on Issue #2354:
(e.g. I understand there's a Rust frontend in progress?).
wave
Ah, I see :-)
By the way I noticed that you only applied this change to the
cranelift-toolscrate, not Wasmtime or Cranelift itself.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).
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:14):cfallin commented on Issue #2354:
@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 :smile:). 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.
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:15):cfallin edited a comment on Issue #2354:
@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.
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:22):uweigand commented on Issue #2354:
@cfallin Thanks!
Wasmtime GitHub notifications bot (Nov 03 2020 at 19:48):bjorn3 commented on Issue #2354:
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).
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.
Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes?
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.
Wasmtime GitHub notifications bot (Nov 04 2020 at 14:41):uweigand commented on Issue #2354:
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.
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
impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'module_environment> {
and the translate_memory_grow routine in that FuncEnvironment is then used from cranelift/wasm/src/code_translator.rs to implement the wasm memory.grow primitive.Is that not correct?
Wasmtime GitHub notifications bot (Nov 04 2020 at 15:38):bjorn3 commented on Issue #2354:
crates/cranelift/src/func_environ.rs
I thought it was
cranelift/src/.... My bad. This will indeed apply the change to Wasmtime.
Last updated: Dec 23 2024 at 12:05 UTC