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?
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.
cfallin commented on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtension
if 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
uext
andsext
will unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtension
unconditionally results in extend ops (fromABIMachineSpec::gen_extend()
) ifUext
orSext
: 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-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 auext
attribute. 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?
cfallin edited a comment on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtension
if 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
uext
andsext
will unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtension
unconditionally results in extend ops (fromABIMachineSpec::gen_extend()
) ifUext
orSext
: 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-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 auext
attribute. 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?
cfallin edited a comment on Issue #2354:
Interesting -- this doesn't match the definition that I had understood (namely, that the
ArgumentExtension
if 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
uext
andsext
will unconditionally result in extend ops:In the new x64 and aarch64 backends, the
ArgumentExtension
unconditionally results in extend ops (fromABIMachineSpec::gen_extend()
) ifUext
orSext
: 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-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 auext
attribute. Is the concern for other frontends, with the assumption that they will (also?) add their own sext/uext attributes? (Edit: @bjorn3, how doescg_clif
handle 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?
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.
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
Baldrdash
ABI, 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?
bjorn3 commented on Issue #2354:
I went through all mentions of
ArgumentExtension
on 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
.
bjorn3 edited a comment on Issue #2354:
I went through all mentions of
ArgumentExtension
on 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()
.
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.
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 ...
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-tools
crate, not Wasmtime or Cranelift itself.
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-tools
crate, 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).
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.
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.
uweigand commented on Issue #2354:
@cfallin Thanks!
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.
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?
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: Nov 22 2024 at 17:03 UTC