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

fix and enable wasm on xplat #4531

Merged
merged 5 commits into from
Jan 12, 2018

Conversation

MikeHolman
Copy link
Contributor

@MikeHolman MikeHolman commented Jan 11, 2018

fix issues with calling convention: we are using stack based calling convention, so we can cleanup a lot of code that was complicated by xmm registers.

fix issue where JIT was calling wrong method for floorf and ceilf

Resolves #3561

@MikeHolman MikeHolman force-pushed the fixwasmxplat branch 2 times, most recently from 269eb42 to f1302fa Compare January 11, 2018 03:52
@MikeHolman MikeHolman changed the title fix wasm on xplat fix and enable wasm on xplat Jan 11, 2018
@MikeHolman MikeHolman force-pushed the fixwasmxplat branch 2 times, most recently from b05cfa6 to e9b98da Compare January 11, 2018 20:48
MikeHolman and others added 2 commits January 11, 2018 12:55
…uncation.

Fix some unit tests to be able to run in xplat builds (mostly problems with filename casing)
Use HeapNew & HeapDelete for exception message in Wasm.
Make test regress.js more generic to be able to add more regression tests there.
@@ -6,7 +6,7 @@
/* global assert,testRunner */ // eslint rule
WScript.Flag("-WasmSignExtends");
WScript.Flag("-WasmI64");
WScript.LoadScriptFile("../UnitTestFrameWork/UnitTestFrameWork.js");
WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

The change for the casing is necessary, however, we should still use forward slashes

@@ -4,7 +4,7 @@
//-------------------------------------------------------------------------------------------------------

/* global assert,testRunner */ // eslint rule
WScript.LoadScriptFile("../UnitTestFrameWork/UnitTestFrameWork.js");
WScript.LoadScriptFile("../UnitTestFramework/UnitTestFramework.js");
WScript.LoadScriptFile("../wasmspec/testsuite/harness/wasm-constants.js");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of wasmspec use WasmSpec or rename the folder.

@obastemur
Copy link
Collaborator

I'm happy that we are about to enable wasm on xplat! @MikeHolman @Cellule Thanks for this!

couple of small comments

LGTM!

@Cellule
Copy link
Contributor

Cellule commented Jan 11, 2018

:lgtm:

Nice fixes! The calling convention looks so much simpler now :)

Reviewed 9 of 11 files at r1, 7 of 9 files at r2, 1 of 2 files at r3, 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@@ -522,6 +524,8 @@
<default>
<files>MathBuiltinsCall.js</files>
<baseline>MathBuiltinsCall.baseline</baseline>
<!-- mac gives has some test which gives different precision result, so exclude it -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: open an issue to track these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can, but do you think it's worth tracking? this is difference in precision with result of atan, which spec says is implementation defined approximation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I thought only some tests have difference but others don't. If this is not the case, well, you are right, no point tracking it!

Generally speaking;
What we did with some of the date time and number formatting testing was to enable-disable per platform. i.e. CH supports the line below that checks if the system running the test is a macOS.

var isMac = (WScript.Platform && WScript.Platform.OS == 'darwin');

@chakrabot chakrabot merged commit 0ab89e1 into chakra-core:release/1.8 Jan 12, 2018
chakrabot pushed a commit that referenced this pull request Jan 12, 2018
Merge pull request #4531 from MikeHolman:fixwasmxplat

fix issues with calling convention: we are using stack based calling convention, so we can cleanup a lot of code that was complicated by xmm registers.

fix issue where JIT was calling wrong method for floorf and ceilf

Resolves #3561
chakrabot pushed a commit that referenced this pull request Jan 12, 2018
Merge pull request #4531 from MikeHolman:fixwasmxplat

fix issues with calling convention: we are using stack based calling convention, so we can cleanup a lot of code that was complicated by xmm registers.

fix issue where JIT was calling wrong method for floorf and ceilf

Resolves #3561
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.

4 participants