Stream: git-wasmtime

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

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.

• [ ] 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 (Aug 05 2020 at 21:23):

jlb6740 requested bnjbvr for a review on PR #2101.

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:23):

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

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:24):

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

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:42):

bjorn3 created PR Review Comment:

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

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:53):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:53):

jlb6740 created PR Review Comment:

@bjorn3 Thanks ... will update this.

Wasmtime GitHub notifications bot (Aug 05 2020 at 21:54):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Aug 05 2020 at 22:51):

abrown submitted PR Review.

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.

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.

• [ ] 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 (Aug 06 2020 at 00:06):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 06 2020 at 00:06):

jlb6740 created PR Review Comment:

:+1: Updated

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.

• [ ] 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 (Aug 07 2020 at 00:31):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 00:31):

cfallin submitted PR Review.

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?

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:08):

jlb6740 submitted PR Review.

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");

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:09):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:11):

jlb6740 edited PR Review Comment.

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.

• [ ] 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 (Aug 07 2020 at 01:17):

jlb6740 submitted PR Review.

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.

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:18):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:19):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Aug 07 2020 at 01:41):

cfallin submitted PR Review.

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.

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.

• [ ] 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 (Aug 07 2020 at 05:25):

jlb6740 merged PR #2101.

Last updated: Nov 22 2024 at 16:03 UTC