Stream: git-wasmtime

Topic: wasmtime / PR #2151 Adds i64x2.mul for the new backend ta...

Wasmtime GitHub notifications bot (Aug 20 2020 at 18:10):

jlb6740 opened PR #2151 from add-i64x2.mul 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 20 2020 at 18:11):

jlb6740 requested bnjbvr for a review on PR #2151.

Wasmtime GitHub notifications bot (Aug 20 2020 at 18:12):

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

Wasmtime GitHub notifications bot (Aug 20 2020 at 18:12):

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

Wasmtime GitHub notifications bot (Aug 20 2020 at 19:33):

jlb6740 updated PR #2151 from add-i64x2.mul 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 28 2020 at 20:54):

jlb6740 updated PR #2151 from add-i64x2.mul 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 28 2020 at 20:55):

jlb6740 has marked PR #2151 as ready for review.

Wasmtime GitHub notifications bot (Aug 28 2020 at 20:55):

jlb6740 requested bnjbvr for a review on PR #2151.

Wasmtime GitHub notifications bot (Aug 28 2020 at 20:55):

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

Wasmtime GitHub notifications bot (Aug 28 2020 at 20:55):

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

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr created PR Review Comment:

nit: here and below, for shift operations, can you use >> (respectively << for left shifts), please? I think it is a bit more usual for pseudo-code.

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr created PR Review Comment:

nit: can you add a space after the comment opening slash, before "The", please?

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr created PR Review Comment:

It is an error to reuse an input register and write into it; the RHS might be reused after this particular CLIF instruction, and there's no indication to regalloc that it ought to be preserve it. Instead, we need to copy RHS into a temporary, and use that, here. Note the vreg copy might not become a copy machine instruction, since regalloc may coalesce those two vregs.

Also, rhs is a RegMem, so it can be either a register or memory, and here the to_reg().unwrap() assumes it's in a register. When defining rhs, you need to use input_to_reg and not input_to_reg_mem to reflect that you want it to be in a register.

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr created PR Review Comment:

Can I recommend to also use the actual register names for the summary, here, that is: dst when it's used, in place of A, etc.?

Wasmtime GitHub notifications bot (Sep 02 2020 at 15:38):

bnjbvr created PR Review Comment:

This is the only use of lhs_tmp2, and at this point we could still reuse lhs, so we should be able to avoid the lhs_tmp2 temporary at this point.

Wasmtime GitHub notifications bot (Sep 04 2020 at 07:02):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 04 2020 at 07:02):

jlb6740 created PR Review Comment:

Absolutely .. :+1:

Wasmtime GitHub notifications bot (Sep 04 2020 at 07:02):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 04 2020 at 07:02):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Sep 07 2020 at 02:49):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 07 2020 at 05:56):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 07 2020 at 06:55):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 07 2020 at 06:56):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 07 2020 at 15:12):

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

Wasmtime GitHub notifications bot (Sep 08 2020 at 09:33):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 08 2020 at 09:33):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 08 2020 at 09:33):

bnjbvr created PR Review Comment:

In addition to the above comment (which I still think would be nice to address, please :)), I have another suggestion that would spare one right shift.

The current decomposition does (Ah * Bl) << 32 + (Al * Bh) << 32 + Al * Bl. I would suggest to factor out the << 32, so have (Ah * Bl) + (Al * Bh) and then apply the left shift to this; it would spare one left shift operation. Does it make sense? (This is how Spidermonkey does it, fwiw.)

Wasmtime GitHub notifications bot (Sep 09 2020 at 05:29):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 09 2020 at 05:29):

jlb6740 created PR Review Comment:

Hi @bnjbvr First of all note, in the latest push yesterday I switched which input referred to the lhs and which referred to the rhs as that was not consistent/correct. For this specific comment what is now the lhs needed the input to be input_to_reg_mem because of the Pmuludq instructions. Specifically https://github.com/bytecodealliance/wasmtime/blob/c2af20b113e509a603db78740cb146ea3fb2246b/cranelift/codegen/src/isa/x64/lower.rs#L599 and https://github.com/bytecodealliance/wasmtime/blob/c2af20b113e509a603db78740cb146ea3fb2246b/cranelift/codegen/src/isa/x64/lower.rs#L610. Am I missing understanding something here?

Wasmtime GitHub notifications bot (Sep 09 2020 at 10:25):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 09 2020 at 10:25):

bnjbvr created PR Review Comment:

input_to_reg_mem means that the input will be put into a register or a memory operand, and we don't know which one. So we can't use to_reg().unwrap() on the result of an input_to_reg_mem call, because the operand might be in memory, and to_reg() returns None in this case. Instead, you want to use input_to_reg, which will always return a register operand, and create a RegMem::reg(...) operand when you need to use it for an instruction that requires a RegMem. Does it help?

Wasmtime GitHub notifications bot (Sep 10 2020 at 07:16):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 10 2020 at 07:16):

jlb6740 created PR Review Comment:

Ok .. And I see you've renamed this :-). So I am still a little uncertain about the reuse of input registers .. is it at any time to use them as a temporary output? I've reworked the algorithm to factor out that shift and modified some things without using too many temps but it's not clear to me if what I have in this update I am about to push is the optimal usage.

Wasmtime GitHub notifications bot (Sep 10 2020 at 07:19):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 10 2020 at 07:22):

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

Wasmtime GitHub notifications bot (Sep 10 2020 at 07:24):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 11 2020 at 13:48):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 11 2020 at 13:48):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 11 2020 at 13:48):

bnjbvr created PR Review Comment:

I think I was the one who suggested using the dst register as a temporary, but thinking about it: sorry, we can't do this. It might be that lhs will get the same physical register as dst (if lhs's final use is for this instruction), thus this move would clobber lhs before it's used in the next pmuludq instruction. Instead, you'll need to use a temporary in place of dst here and in the next instruction; I think you can reuse rhs_1 though, since it's dead at this point. (This might require a final gen_move(dst, rhs_1, ty)) after the whole sequence, which is fine too.)

Wasmtime GitHub notifications bot (Sep 11 2020 at 13:48):

bnjbvr created PR Review Comment:

(here's the second time)

Wasmtime GitHub notifications bot (Sep 11 2020 at 13:48):

bnjbvr created PR Review Comment:

nit: here and below, this can be simplified to lhs_1 instead of the whole expression (lhs_1.to_reg() returns a non-writable register, and Writable::from_reg makes it writable again; the initial lhs_1 is writeable).

Wasmtime GitHub notifications bot (Sep 11 2020 at 18:50):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 11 2020 at 18:50):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 11 2020 at 18:50):

jlb6740 created PR Review Comment:

Makes sense. Thanks!

Wasmtime GitHub notifications bot (Sep 11 2020 at 19:28):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 11 2020 at 19:39):

jlb6740 updated PR #2151 from add-i64x2.mul 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 (Sep 11 2020 at 20:17):

jlb6740 merged PR #2151.

Last updated: Nov 22 2024 at 16:03 UTC