Stream: git-wasmtime

Topic: wasmtime / PR #5893 cranelift: Add big and little endian ...

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:13):

jan-justin opened PR #5893 from cranelift-big-little-endian-access to main:

This PR aims to add specific big-endian and little-endian memory accesses to the interpreter when performing a load or store operation, as discussed over at #5881.

<!--

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 (Feb 28 2023 at 15:16):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:16):

jan-justin created PR review comment:

As far as I can see, this function is only ever used for testing purposes so I assumed that specific endianness is not an issue here, but do let me know.

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:19):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:19):

jan-justin created PR review comment:

@afonso360 you posited theses functions more as a question, so let me know if you don't think it will suffice.

Also, this could probably be DRY'd up with a macro, but my macrofu is not there yet. Let me know if I should go and do it though.

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:32):

jan-justin updated PR #5893 from cranelift-big-little-endian-access to main.

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:12):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:12):

afonso360 created PR review comment:

Yeah, mostly because they match what we have in the rust standard library so it seemed natural to keep that naming convention.

This does involve a lot of repetitive code, and I also don't see a way out of this without creating a macro. @jameysharp any ideas here?

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:13):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:13):

afonso360 created PR review comment:

That seems to be used by our trampolines, so _ne should be the correct choice!

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:13):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:21):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 18:27):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:03):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:03):

jameysharp created PR review comment:

Huh, yeah, this is tricky! As much as I dislike the duplication, I suspect a macro won't make the code more clear.

One question first: why aren't vectors byte-swapped in this implementation? Is there something I'm missing that makes them special?

Here's an alternative way to split this up. I'm not claiming it's the right choice, but it's an alternative we can discuss.

First, write a function that takes a DataValue and returns a DataValue, but with its contents unconditionally byte-swapped. In other words, if you have a DataValue::I32(i), return DataValue::I32(i.swap_bytes()). For vectors, maybe we should be calling reverse on the arrays?

Next, write a pair of functions on DataValue, where one calls the above byte-swap function when run on a big-endian system, and the other calls it when run on a little-endian system. See the example code in u32::to_be and to_le for the pattern to follow. Note that to_be and from_be are functionally identical (both functions only byte-swap on little-endian systems), and similarly for to_le and from_le. So we don't actually need to and from variants for each endianness; just one will do.

Finally, keep write_to_slice_ne and read_from_slice_ne as they are here. The _le variants should call the _ne variants, but pass the DataValue through the method that only byte-swaps on big-endian systems. Similarly, the _be variants call the method that byte-swaps on little-endian systems. When reading, the swap happens after the native-endian read; when writing, the swap happens before the native-endian write.

That reduces this from six copies of this match on all of DataValue's variants, to three. That's still two more than I'd prefer, but I think it's pretty reasonable.

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:39):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:39):

jameysharp created PR review comment:

For future work, not part of this PR: @afonso360, can/should checked_load validate mem-flags like aligned, now that they're passed in here anyway? And same question for checked_store.

Wasmtime GitHub notifications bot (Mar 01 2023 at 09:56):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 09:56):

jan-justin created PR review comment:

Thank you for the input!

No, vectors are not special. It seems I just slipped up. Thanks for pointing it out.

As for your proposal, I would be inclined to agree with you. I am in favour of cutting it down so as to make it more manageable and catch errors like the one above.

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:54):

afonso360 created PR review comment:

Oh yeah it absolutely should! I've opened #5899 for this

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:54):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:55):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:06):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:06):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Mar 01 2023 at 19:10):

jan-justin updated PR #5893 from cranelift-big-little-endian-access to main.

Wasmtime GitHub notifications bot (Mar 01 2023 at 19:22):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 19:22):

jan-justin created PR review comment:

I used the Rust core lib as inspiration, and modelled it quite closely. However, I am unsure about whether to change the write_to_slice_* and read_from_slice_* methods to move instead of borrow. The result is having to copy data in the write calls. Let me know if this is not desirable.

Wasmtime GitHub notifications bot (Mar 01 2023 at 19:31):

jan-justin edited PR review comment.

Wasmtime GitHub notifications bot (Mar 01 2023 at 19:35):

jan-justin edited PR review comment.

Wasmtime GitHub notifications bot (Mar 02 2023 at 02:22):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 02 2023 at 02:22):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 02 2023 at 02:22):

jameysharp created PR review comment:

I have a slight preference for writing this in the if cfg!(...) { ... } else { ... } style because that way the compiler will check that both branches are valid before constant-folding one of them away. Also it's shorter that way.

Wasmtime GitHub notifications bot (Mar 02 2023 at 02:22):

jameysharp created PR review comment:

I didn't think about needing to clone! But I think the way you've done it is fine. For one thing, this is only 32 bytes if I counted correctly, so it's not that much to copy. LLVM might even optimize away the copies. But more importantly, this is primarily used in the interpreter, which is already not expected to be super fast.

Wasmtime GitHub notifications bot (Mar 02 2023 at 07:51):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 02 2023 at 07:51):

jan-justin created PR review comment:

No problem. Consider it done.

Wasmtime GitHub notifications bot (Mar 02 2023 at 07:53):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 02 2023 at 07:53):

jan-justin created PR review comment:

Great, thanks again!

I figured it may not be that big a deal, but I drew attention to it just in case. I will keep this in mind going forward though.

Wasmtime GitHub notifications bot (Mar 02 2023 at 07:57):

jan-justin updated PR #5893 from cranelift-big-little-endian-access to main.

Wasmtime GitHub notifications bot (Mar 02 2023 at 12:31):

afonso360 merged PR #5893.

Last updated: Nov 22 2024 at 17:03 UTC