Support explicit endianness in Cranelift IR MemFlags #2492
Conversation
|
This is alternative approach to PR #2488 -- see also the discussion there. |
github-actions
bot
added
cranelift
There was a problem hiding this comment.
Thanks for this! Approving this PR rather than #2488 after discussion today in the Cranelift call. For the record, the decision was to allow this limited non-determinism (more precisely, machine-specific semantics) of CLIF, i.e. native-endian memory accesses, in order to permit code generators to produce performant CLIF (with native endianness) without knowing the target machine. It seems to me that that "tailor the CLIF to the target" requirement is more at odds with a clean design than the "endianness depends on target" behavior, and NaN canonicalization (or the ability to turn it off) is precedent for target-specific behavior rather than target-specific CLIF. But we can always revisit this later if we need to.
cranelift/codegen/src/ir/memflags.rs
Outdated
| /// In addition, the flags determine the endianness of the memory access. As opposed to the | ||
| /// optimization flags defined above, modifying the endianness would always change program | ||
| /// semantics, therefore the endianness must always be explicitly specified when constructing | ||
| /// a MemFlags value, and cannot be changed later. |
There was a problem hiding this 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).
cranelift/codegen/src/ir/memflags.rs
Outdated
| } | ||
| None => false, | ||
| } | ||
| } | ||
|
|
||
| /// Return endianness of the memory access. |
There was a problem hiding this 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.
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.
|
Updated comments. |
|
This doesn't actually implement handling different endianness in the backends. |
You can look at the new s390x backend, which does implement both native (big-endian) and little-endian accesses. On little-endian platforms there is normally no need to implement big-endian access. |
|
Big-endian access would be useful for #2953 (comment) There is currently no |
Well, I guess someone familiar with the other platforms could implement big-endian memory access support for the other back-ends. I'm not sure if there are direct big-endian load/store instructions on all other platforms cranelift supports; where there are none, the back-end would have to fall back to bswap or equivalent to implement the IR semantics. |
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.