Stream: git-wasmtime

Topic: wasmtime / PR #2101 Add basic packed integer arithmetic


view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:23):

jlb6740 opened PR #2101 from add-basic-packed-integer-arithmetic to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:23):

jlb6740 requested bnjbvr for a review on PR #2101.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:23):

jlb6740 requested abrown and bnjbvr for a review on PR #2101.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:24):

jlb6740 requested cfallin, abrown and bnjbvr for a review on PR #2101.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 created PR Review Comment:

You could simplify this to ty.lane_count() > 1.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:53):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:53):

jlb6740 created PR Review Comment:

@bjorn3 Thanks ... will update this.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 21:54):

jlb6740 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 22:51):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 05 2020 at 22:51):

abrown created PR Review Comment:

I've been using ty.is_vector() && ty.bits() == 128 on the FP SIMD side.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 06 2020 at 00:06):

jlb6740 updated PR #2101 from add-basic-packed-integer-arithmetic to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 06 2020 at 00:06):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 06 2020 at 00:06):

jlb6740 created PR Review Comment:

:+1: Updated

view this post on Zulip Wasmtime GitHub notifications bot (Aug 06 2020 at 18:23):

jlb6740 updated PR #2101 from add-basic-packed-integer-arithmetic to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 00:31):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 00:31):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 00:31):

cfallin created PR Review Comment:

This is a bit surprising -- it looks like below, int_ty_is_64 is only called in the non-vector case, and the panic message below also implies that it's only really applicable to the scalar case. I might be missing something though?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:08):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:08):

jlb6740 created PR Review Comment:

Ahh yes. I didn't want to add these explicitly here. I am thinking we should just:

types::I64 => true, _ => panic!("type {} is not I64");

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:09):

jlb6740 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:11):

jlb6740 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:14):

jlb6740 updated PR #2101 from add-basic-packed-integer-arithmetic to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:17):

jlb6740 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:17):

jlb6740 created PR Review Comment:

I think the intent of the message is to catch if the type isn't int, but I don't know how important that is if the function is only called from one place and is this straight forward.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:18):

jlb6740 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:19):

jlb6740 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:41):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 01:41):

cfallin created PR Review Comment:

Hmm, yeah, it did serve as a sanity-check (that e.g. we don't have a float type here), but it seems we don't consistently verify types everywhere, and in any case such a problem would be caught by both (i) the CLIF verifier and (ii) asserts/panics at instruction-emission time. So if you want to simply replace it with ty.unwrap() == types::I64 at the one callsite, I think that's fine.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 04:48):

jlb6740 updated PR #2101 from add-basic-packed-integer-arithmetic to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2020 at 05:25):

jlb6740 merged PR #2101.


Last updated: Dec 23 2024 at 12:05 UTC