Stream: git-wasmtime

Topic: wasmtime / PR #3084 Add simd_extmul_* support for x64


view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2021 at 01:13):

jlb6740 opened PR #3084 from implement_ext_mul_x64 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 (Jul 14 2021 at 01:37):

jlb6740 updated PR #3084 from implement_ext_mul_x64 to main.

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

jlb6740 updated PR #3084 from implement_ext_mul_x64 to main.

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

jlb6740 updated PR #3084 from implement_ext_mul_x64 to main.

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

jlb6740 has marked PR #3084 as ready for review.

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

jlb6740 requested abrown for a review on PR #3084.

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

jlb6740 requested akirilov-arm for a review on PR #3084.

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

abrown submitted PR review.

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

abrown created PR review comment:

Initially I thought that this matching was too greedy--that it would match sequences that it shouldn't be and then crash because the types are wrong. I think now that it is OK because swiden_high only really allows the following types: I8X16, I16X8, I32X4. I think it might be worthwhile to mention this assumption in some comments at the top so that future readers aren't confused.

v0 = swiden.i64

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

abrown created PR review comment:

Not sure why swiden_input is needed: couldn't the InsnInputs be created inline?

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

abrown created PR review comment:

I think the order of the lowerings should be reversed: scalar lowerings first, then vector lowerings, then pattern matching lowerings. As it stands in this PR, every imul.i64 is going to require a bunch of matching calls and type comparisons before it ever gets lowered. I think scalar multiplication is going to be the most common, then plain vector multiplication, so I would think we would want to make those common paths shorter. (Not sure exactly how much this affects compile time but hopefully you see what I mean).

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

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 04:24):

jlb6740 created PR review comment:

Sure .. I agree. That was my original thought but I never reverted the initial implementation.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 04:24):

jlb6740 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 06:50):

jlb6740 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 07:05):

jlb6740 updated PR #3084 from implement_ext_mul_x64 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 07:09):

jlb6740 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 07:09):

jlb6740 created PR review comment:

Also note, the code must remain at the top as the fall through target since I am using "if let Some(_) .." to check for op sources and there is no if not let Some(_) ..". Still there is just one branch to get to the other lowerings.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 07:31):

jlb6740 requested abrown for a review on PR #3084.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 08:07):

jlb6740 merged PR #3084.


Last updated: Dec 23 2024 at 13:07 UTC