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

Feat wasm bigint #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feat wasm bigint #1

wants to merge 1 commit into from

Conversation

littledan
Copy link

Creating PR to make review comments

index,
RelocInfo::WASM_STUB_CALL)
: jsgraph()->HeapConstant(
BUILTIN_CODE(isolate_, WasmToBigIntJavaScript));
Copy link
Author

Choose a reason for hiding this comment

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

Dumb question (I haven't worked with CSA before): Rather than calling this runtime stub, would it work to call CodeStubAssembler::BigIntFromInt64 directly, or write code that's similar to BuildChangeInt32ToTagged to do accomplish this task?

Copy link
Owner

Choose a reason for hiding this comment

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

Your question make sense and it would be possible in JS, wasm has a additionnal restriction for calling its runtime stubs, see https://github.com/xtuc/v8/pull/1/files/dda86c1f2fc8c7dfee946527154ae5cccffde6a4#diff-74de4cf1fe91d141c93cad45c7cb81e0R4357. It's more or less a workarround.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, is BuildChangeInt32ToTagged a runtime stub, or just a bunch of compiler IR that gets generated inline? I thought it was the latter, and that's what I'm suggesting be done here.

Copy link
Owner

Choose a reason for hiding this comment

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

BuildChangeInt32ToTagged generates IR, what you're suggesting is interesting, it could avoid the additional procedure call. I think that I could generate the IR directly too, thanks for your suggestion!

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure how to do because I'd like to use the code in BigIntFromInt64 on the CSA. BuildChangeInt32ToTagged is a bit simplier and probably not existing already on the CSA.

Copy link
Owner

@xtuc xtuc Nov 7, 2018

Choose a reason for hiding this comment

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

How I implemented is consistent with the other wasm buitilns or trampolines to builtin. Howerver, the code structure is wrong.

Copy link
Author

Choose a reason for hiding this comment

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

I think it'd be OK to go down either of these paths, as BigInts have significant performance overhead anyway (though my personal preference would be the faster one--maybe you can work out how to share the code).

Copy link
Owner

Choose a reason for hiding this comment

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

I though about that also, the conversion only happen at the JS boudaries. In wasm it's a unboxed int64 type.

Howerver, I'd like to make it right first.

@xtuc xtuc force-pushed the feat-wasm-bigint branch from dda86c1 to 95ac1e9 Compare November 6, 2018 15:43
@xtuc xtuc force-pushed the feat-wasm-bigint branch 2 times, most recently from 2fa1350 to 85eadb4 Compare November 20, 2018 08:48
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