wasmtime / Issue #2488 Make endianness explicit in Cranel... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #2488 Make endianness explicit in Cranel...

Wasmtime GitHub notifications bot (Dec 09 2020 at 12:40):

By defining the endianness in a ternary way (with native as an option) I think we can keep the text format and builder interface backward-compatible; we can accept the new big/little options in the text format, and we can accept MemFlags with explicit endianness if the client desires. But current code doesn't break or regress in performance. @uweigand has done the hard part of plumbing flags through, so we shouldn't drop them once they reach the backend; and in cranelift-wasm we can carefully audit codegen to ensure we specify little-endianness.

Well, I'd be happy to implement that option as well. Just one question to clarify: when adding a native option, should that have to be specified explicitly, i.e. something like MemFlags::new(Endianness::Native) or should we go back to allowing plain MemFlags::new() which would implicitly mean native endianness?

Wasmtime GitHub notifications bot (Dec 09 2020 at 13:11):

bjorn3 commented on Issue #2488:

MemFlags::new() would look nicer, but MemFlags::new(Endianness::Native) would make it harder for optimizations to use the wrong endianness.

Wasmtime GitHub notifications bot (Dec 09 2020 at 15:40):

uweigand commented on Issue #2488:

I've created a new PR #2492 that implements optional endianness markers in MemFlags. This is the MemFlags::new() approach for now -- obviously that results in a drastically smaller patch.

Let me know if you'd prefer the MemFlags::new(Endianness::Native) version instead.

Wasmtime GitHub notifications bot (Dec 09 2020 at 16:50):

cfallin commented on Issue #2488:

Thanks @uweigand -- I do like the alternative (with-native-option) approach better, I think; anyone else. have thoughts on this (@sunfishcode has thought about determinism I know; @julian-seward1 and maybe others in the last CL call)?

Wasmtime GitHub notifications bot (Dec 14 2020 at 22:00):

cfallin commented on Issue #2488:

Closed after merging the alternative #2492 (thanks for doing the work on both options and the patience during review!).

Last updated: Nov 22 2024 at 16:03 UTC