Stream: git-wasmtime

Topic: wasmtime / PR #2953 Cranelift: Add `LibCall::Memcmp`

Wasmtime GitHub notifications bot (May 31 2021 at 19:28):

scottmcm opened PR #2953 from add-memcmp to main:

The comment says the enum is "likely to grow" and the function's been in libc since C89, so hopefully this is ok.

I'd like to use it for emitting things like array equality.

<!--

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 (May 31 2021 at 19:36):

scottmcm edited PR #2953 from add-memcmp to main:

This adds LibCall::Memcmp and a corresponding FunctionBuilder::call_memcmp, following the existing patterns of memcpy, memset, & memmove. The comment says the enum is "likely to grow" and the memcmp's been in libc since C89, so hopefully this is ok.

I'd like to have this for emitting equality tests of large, simple no-padding types, like [u32; 123].

I don't know who should review this.

Wasmtime GitHub notifications bot (Jun 01 2021 at 15:42):

scottmcm updated PR #2953 from add-memcmp to main.

Wasmtime GitHub notifications bot (Nov 29 2021 at 09:59):

scottmcm updated PR #2953 from add-memcmp to main.

Wasmtime GitHub notifications bot (Nov 29 2021 at 10:08):

scottmcm updated PR #2953 from add-memcmp to main.

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:01):

cfallin created PR review comment:

This could probably become a helper on Type, perhaps Type::int_with_byte_size(...) (and create a corresponding Type::int_with_bit_size(...) and maybe the float and bool equivalents too).

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:35):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:35):

scottmcm created PR review comment:

Hmm, Type::int is already mostly Type::int_with_bit_size, just with a different parameter type.

Here's a few possible directions. Let me know which you'd prefer I do, @cfallin

• Add Type::int_with_byte_size and Type::int_with_bit_size, concentrating on the "different integer width" use by having them take impl TryInto<u16>.
• Add a Type::int_bytes that takes u16
• Rename Type::int to Type::int_bits and add Type::int_bytes (rg only spots 10 obvious uses of Type::int so it might not be as horrible as I'd have guessed)
• One of the above, but using a type other than u16 for Type::int (TBH u16 is kinda odd there, since the biggest value it cares about is 128 and would fit in a u8, but it's also not the u32 that rust's std tends to use for numbers of bits)
• Some combination thereof
• Make a follow-up PR for one of the above
• Do nothing
  

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 23:46):

cfallin created PR review comment:

Ah, I had missed the existing Type::int() -- IMHO, it might be better to leave that as-is (Type::int(32) is simple/ergonomic/very clear) and then add this as Type::int_with_byte_size() (I slightly prefer this over int_bytes, the latter sounds like it's a special kind of int that's a vector of bytes or something like that). And I guess let's match the u16 for consistency (I'm not sure why u16, maybe to allow 256-bit vecs but still be somewhat compact?).

Thanks!

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:22):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:22):

scottmcm created PR review comment:

I'm not sure why u16, maybe to allow 256-bit vecs but still be somewhat compact?

That would make sense if it was stored, but Type is just pub struct Type(u8);, so it's just a parameter, and thus the choice seems like it could be pretty much anything.

I'll go add Type::int_with_byte_size(u16) -> Type as requested.

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:48):

scottmcm updated PR #2953 from add-memcmp to main.

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:54):

scottmcm updated PR #2953 from add-memcmp to main.

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:10):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:10):

cfallin created PR review comment:

OK, yeah, that existing bit is unclear then; but, better saved for a later cleanup :-) Thanks!

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 02:36):

cfallin merged PR #2953.

Last updated: Nov 22 2024 at 17:03 UTC