Stream: git-wasmtime

Topic: wasmtime / PR #3351 cranelift: Add support for parsing i1...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2021 at 14:40):

afonso360 opened PR #3351 from parser-i128 to main:

Hey,

This PR adds support for parsing i128 data values, it also cleans up the i128 test suite to pass those values directly to the functions. (We previously passed the bottom and top half in i64's and iconcat them together)

This causes a bunch of noise in the diff (sorry!). I've separated this into two commits, the first one with the parser changes, and the second one with test suite changes. It might be easier to review it that way.

Since we now pass i128's directly we need to enable the enable_llvm_abi_extensions flag. This is because the windows_fastcall ABI does not specify how to pass i128 values to functions. This is only necessary for the tests to pass on x86_64 windows.

It doesn't add support for iconst.i128 since its instruction format (UnaryImm) doesen't allow us to include a 128 bit value. We would need to change it to UnaryConst for that.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2021 at 15:12):

afonso360 updated PR #3351 from parser-i128 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 19:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 19:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 19:46):

cfallin created PR review comment:

Why is this different (ne rather than le)? It would be useful to have a comment here explaining why we need to use native endian rather than explicit little endian as in the other cases.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 19:46):

cfallin created PR review comment:

I see the duplication is pre-existing, but it's also unfortunate to continue it here. While it may expand the scope of this PR a bit, I wonder if there's a way to write a single polymorphic implementation? Maybe it can take the signed/unsigned types and require FromStr / FromHex / whatever encapsulates wrapping_neg?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 21:10):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 21:10):

afonso360 created PR review comment:

We actually do need ne in the other ones (for big endian systems), however changing le -> ne is already in a separate PR (#3329).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 22:15):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2021 at 22:15):

cfallin created PR review comment:

Ah, yes, this makes sense. Thanks for the nudge -- I re-read the issue and decided to merge #3329 to resolve the immediate regression on s390x (so this line now matches what's on main), and created a new issue to track the broader question of endianness in the interpreter.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2021 at 14:02):

afonso360 updated PR #3351 from parser-i128 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2021 at 14:04):

afonso360 updated PR #3351 from parser-i128 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2021 at 14:08):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2021 at 14:08):

afonso360 created PR review comment:

I tried doing this as a generic function, but it was proving to be somewhat difficult, most of the functions that we need / could use are not in separate traits, but as functions associated with the primitive types. FromStr works, but FromHex doesen't exist. Casting between signed and unsigned types in a generic function, as far as I can tell is a no go.

We could solve pretty much all of these problems by adding a dependency on the num crate, but I don't think this justifies it.

I've implemented this as a macro, hope that is ok.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2021 at 11:12):

afonso360 requested cfallin for a review on PR #3351.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2021 at 17:04):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2021 at 17:04):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2021 at 17:04):

cfallin created PR review comment:

Perfect, thank you!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2021 at 17:04):

cfallin merged PR #3351.

Last updated: Dec 23 2024 at 12:05 UTC