Wasmtime GitHub notifications bot (Dec 09 2020 at 15:30): Wasmtime GitHub notifications bot (Dec 09 2020 at 15:30):uweigand opened PR #2492 from endian-memory-v5 to main:
WebAssembly memory operations are by definition little-endian even on
big-endian target platforms. However, other memory accesses will require
native target endianness (e.g. to access parts of the VMContext that is
also accessed by VM native code). This means on big-endian targets,
the code generator will have to handle both little- and big-endian
memory accesses. However, there is currently no way to encode that
distinction into the Cranelift IR that describes memory accesses.This patch provides such a way by adding an (optional) explicit
endianness marker to an instance of MemFlags. Since each Cranelift IR
instruction that describes memory accesses already has an instance of
MemFlags attached, this can now be used to provide endianness
information.Note that by default, memory accesses will continue to use the native
target ISA endianness. To override this to specify an explicit
endianness, a MemFlags value that was built using the set_endianness
routine must be used. This patch does so for accesses that implement
WebAssembly memory operations.This patch addresses issue #2124.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 14 2020 at 18:47):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Dec 14 2020 at 18:47):cfallin created PR Review Comment:
Add to comment here that if no endianness is specified, then the native endianness is returned? We might also include a small note about why we have this native-endian option: it allows code generators to produce performant CLIF without requiring access to the target machine's endianness.
Wasmtime GitHub notifications bot (Dec 14 2020 at 18:47):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Dec 14 2020 at 18:47):cfallin created PR Review Comment:
Outdated comment, I think?
To the point about endianness at construction: perhaps we could have, still, a separate constructor (used just where explicit, possibly non-native, endianness is needed, e.g. in the Wasm translator). I don't think that ensuring their immutability is a huge concern though: adding other flags can also break program semantics (e.g., promising a value is "readonly" when it's not might break optimizations relying on alias analysis), so I don't think we have to worry too much about distinguishing endianness flags (other than ensuring they're mutually exclusive, as you've done).
Wasmtime GitHub notifications bot (Dec 14 2020 at 19:16):uweigand updated PR #2492 from endian-memory-v5 to main:
WebAssembly memory operations are by definition little-endian even on
big-endian target platforms. However, other memory accesses will require
native target endianness (e.g. to access parts of the VMContext that is
also accessed by VM native code). This means on big-endian targets,
the code generator will have to handle both little- and big-endian
memory accesses. However, there is currently no way to encode that
distinction into the Cranelift IR that describes memory accesses.This patch provides such a way by adding an (optional) explicit
endianness marker to an instance of MemFlags. Since each Cranelift IR
instruction that describes memory accesses already has an instance of
MemFlags attached, this can now be used to provide endianness
information.Note that by default, memory accesses will continue to use the native
target ISA endianness. To override this to specify an explicit
endianness, a MemFlags value that was built using the set_endianness
routine must be used. This patch does so for accesses that implement
WebAssembly memory operations.This patch addresses issue #2124.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 14 2020 at 21:59):cfallin merged PR #2492.
Last updated: Nov 22 2024 at 16:03 UTC