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

jit: support for mul #111

Merged
merged 28 commits into from Jan 3, 2022
Merged

jit: support for mul #111

merged 28 commits into from Jan 3, 2022

Conversation

mathetake
Copy link
Member

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake marked this pull request as ready for review January 2, 2022 06:27
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Copy link
Collaborator

@codefromthecrypt codefromthecrypt 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. I'm starting to notice an opportunity for consistency though it wasn't introduced here. We can think about it now or later.

wasm/jit/jit_amd64.go Show resolved Hide resolved
wasm/jit/jit_amd64.go Show resolved Hide resolved
wasm/jit/jit_amd64.go Show resolved Hide resolved
wasm/jit/jit_amd64.go Show resolved Hide resolved
wasm/jit/jit_amd64.go Outdated Show resolved Hide resolved
wasm/jit/jit_amd64.go Show resolved Hide resolved
wasm/jit/jit_amd64_test.go Show resolved Hide resolved
wasm/jit/jit_amd64_test.go Outdated Show resolved Hide resolved
wasm/jit/jit_amd64_test.go Outdated Show resolved Hide resolved
wasm/jit/jit_value_location_amd64.go Outdated Show resolved Hide resolved
mathetake and others added 19 commits January 3, 2022 17:45
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Co-authored-by: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@codefromthecrypt
Copy link
Collaborator

codefromthecrypt commented Jan 3, 2022 via email

Copy link
Collaborator

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

approve!

@@ -781,6 +783,129 @@ func (c *amd64Compiler) compileSub(o *wazeroir.OperationSub) error {
return nil
}

// compileMul adds instructions to multiply two operands which may be on the stack or registers.
// After execution, the result of multiplication pushed onto the stack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

that the prior RDX is pushed on the stack.. is that intuitive enough to assume as being a result of multiplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually RDX is not pushed on the stack, RDX holds the overflow info according to the manual, but we don't need to use it in Wasm convention

@mathetake mathetake merged commit 76f9e67 into main Jan 3, 2022
@mathetake mathetake deleted the mul branch January 3, 2022 11:06
@mathetake
Copy link
Member Author

@codefromthecrypt thanks for the thorough review!

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.

None yet

2 participants