Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spec] Fix extmul & extadd execution #1716

Merged
merged 1 commit into from
Jan 3, 2024
Merged

[spec] Fix extmul & extadd execution #1716

merged 1 commit into from
Jan 3, 2024

Conversation

702fbtngus
Copy link
Contributor

At step 11 of the definition of extmul (below image), there is an expression $\text{imul}_{t_2 \times N}(k_1^*, k_2^*)$.
image

First solution

I guess it should be $\text{imul}_{|t_2|} (k_1^*, k_2^*)$.
The underscored argument of $\text{imul}$, which is $t_2 \times N$ is typed wrong. From the definition of $\text{imul}$, its argument should be an integer.
image
Here, from line 12, $k^*$ should be a sequence of length $N$ with its elements typed $t_2$. So I think the argument should be $|t_2|$.
Same problem occurs in the definition of extadd (below image).
image
I think the expression $\text{iadd}_N(j_1, j_2)^*$ at step 6 should be $\text{iadd}_{|t_2|} (j_1, j_2)^*$ instead, and fixed the document.

Second solution

Another solution (which I'm not so sure of) could be using the lane-wise applying notation.
image
With this notation, $\text{lanes}_{t_2 \times N}^{-1}(\text{imul}_{t_2 \times N} (k_1^*, k_2^*))$ from extmul can be replaced by $\text{mul}_{t_2 \times N} (k_1^*, k_2^*)$, and $\text{lanes}_{t_2 \times N}^{-1}(\text{iadd}_N(j_1, j_2)^*)$ from extadd by $\text{add}_{t_2 \times N}(j_1, j_2)^*$.

I'm not sure about the second solution nor which one is better, so I applied the first one to the document for now.

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@rossberg rossberg merged commit 4877171 into WebAssembly:main Jan 3, 2024
4 checks passed
dhil added a commit to wasmfx/specfx that referenced this pull request Jan 25, 2024
* [test] Adjust br_table length test to avoid wrong failure (WebAssembly#1715)

* [spec] Fix extmul & extadd execution (WebAssembly#1716)

* [interpreter] Add dune dependencies (WebAssembly#1717)

* Revert "[interpreter] Add dune dependencies (WebAssembly#1717)"

This reverts commit 44ed035.

* [interpreter/test] Fix inconsistent use of float values in `spectest` (WebAssembly#1718)

* [spec] Fix operand order in reduction of shape.replace_lane (WebAssembly#1721)

* [spec] Fix sx to be sx? in reduction of vcvtop_zero (WebAssembly#1720)

---------

Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: 702fbtngus <79245586+702fbtngus@users.noreply.github.com>
Co-authored-by: Muqiu Han (韩暮秋) <muqiu-han@outlook.com>
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
Co-authored-by: Dongjun Youn / 윤동준 <f52985@gmail.com>
Co-authored-by: Hoseong Lee <101983402+HoseongLee@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants