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

Remove Simd.js code from ChakraCore - Part 1 (ifdef out) #3296

Merged
merged 2 commits into from
Jul 12, 2017

Conversation

agarwal-sandeep
Copy link
Collaborator

Put SIMDjs code in library, language and backend under #ifdef ENABLE_SIMDJS
undef ENABLE_SIMDJS
Some portion of SIMD code (IRType, OpCodesSimd, AsmJsBuiltInNames etc.) is used in WASM simd so it is left as it is. This will be take care of under #3274
Haven't ifdef'd TypeIds_SIMD* as that will make the TypeIds enum messy. During final cleanup we can clean this.

This reduce Chakra.dll x64 release binary size by 0.52 MB and ChakraCore x64 release binary size by 0.43 MB

@agarwal-sandeep
Copy link
Collaborator Author

@LouisLaf @Cellule please take a look

@Cellule
Copy link
Contributor

Cellule commented Jul 11, 2017

@arunetm @Krovatkin fyi

@Krovatkin
Copy link
Collaborator

@Cellule @agarwal-sandeep There's quite a few of ENABLE_SIMDJS in AsmJs-specific files. There's a 99% chance we are going to need that code. If the next step is to disappear all the ENABLE_SIMDJS code, I'd suggest we either use a different ifdef for asmjs stuff or we let it be (the second one is a bit more preferable since it would simplify a merge process for us). If the main goal of this PR to reduce dll memory footprint, asmjs-specific code wouldn't probably account to much comparing to JavaScript API

@Cellule
Copy link
Contributor

Cellule commented Jul 11, 2017

@Krovatkin We want to give some time before actually removing the code so we can more precisely determine what we actually need for wasm.simd. Also, there is a lot of asm.js code that we don't want anymore either, I am thinking mostly of the bytecode generator.

Once we merge this PR, I suggest to integrate master into wasm.simd branch and that way we could start figuring out that code sooner. I would recommend at that time to rename the occurences of #ifdef ENABLE_SIMDJS to #ifdef ENABLE_WASM_SIMD which would help resolve potential conflicts in the future.

@Cellule
Copy link
Contributor

Cellule commented Jul 11, 2017

Reviewed 415 of 415 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


lib/Backend/BailOut.cpp, line 825 at r1 (raw file):

            {
                // SIMD_JS
                simdValue = layout->GetSimdValueAtOffset(offset);

Missed this block


lib/Backend/BailOut.cpp, line 886 at r1 (raw file):

                   )
                {
                    simdValue = *((SIMDValue *)&(registerSaveSpace[offset - 1]));

Missed this block


lib/Backend/BailOut.cpp, line 1040 at r1 (raw file):

#else
            // https://github.com/Microsoft/ChakraCore/issues/3274
            // Change signature of RestoreValue if WASM simd doesn't use this

I doubt wasm simd will need this since we don't have bailouts in wasm. But it's okay to leave this for a later time.


lib/Backend/GlobOpt.cpp, line 16404 at r1 (raw file):

            {
                IR::BailOutKind instrBailoutKind = instr->GetBailOutKind();
#ifdef ENABLE_SIMDJS

Instead of copy/pasting the Assert here could we do

                Assert(
#ifdef ENABLE_SIMDJS
                    instrBailoutKind == IR::BailOutSimd128F4Only ||
                    instrBailoutKind == IR::BailOutSimd128I4Only ||
#endif
                    instrBailoutKind == IR::BailOutIntOnly ||
                    instrBailoutKind == IR::BailOutExpectingInteger ||
                    instrBailoutKind == IR::BailOutOnNotPrimitive ||
                    instrBailoutKind == IR::BailOutNumberOnly ||
                    instrBailoutKind == IR::BailOutPrimitiveButString);

Comments from Reviewable

@Krovatkin
Copy link
Collaborator

@Cellule

Once we merge this PR, I suggest to integrate master into wasm.simd branch and that way we could start figuring out that code sooner.

Sounds like a plan. Though, I'd prefer to merge two more changes (they are pretty stable and cleaned up) into wasm.simd before rebasing it onto master. This way, I won't have to rebase them piecemeal afterwards. It would be a good milestone since 99% of 32x4 ops should have been enabled/added. At that point, I'd also rename some ENABLE_SIMDJS to ENABLE_WASM_SIMD as you are suggesting.

@agarwal-sandeep
Copy link
Collaborator Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


lib/Backend/BailOut.cpp, line 825 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Missed this block

Will leave it for when we change the function signature to remove isSimd* parameters


lib/Backend/BailOut.cpp, line 886 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Missed this block

Will leave it for when we change the function signature to remove isSimd* parameters


lib/Backend/GlobOpt.cpp, line 16404 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Instead of copy/pasting the Assert here could we do

                Assert(
#ifdef ENABLE_SIMDJS
                    instrBailoutKind == IR::BailOutSimd128F4Only ||
                    instrBailoutKind == IR::BailOutSimd128I4Only ||
#endif
                    instrBailoutKind == IR::BailOutIntOnly ||
                    instrBailoutKind == IR::BailOutExpectingInteger ||
                    instrBailoutKind == IR::BailOutOnNotPrimitive ||
                    instrBailoutKind == IR::BailOutNumberOnly ||
                    instrBailoutKind == IR::BailOutPrimitiveButString);

That generates C2121 error


Comments from Reviewable

@Cellule
Copy link
Contributor

Cellule commented Jul 11, 2017

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@chakrabot chakrabot merged commit 00fc551 into chakra-core:release/1.6 Jul 12, 2017
chakrabot pushed a commit that referenced this pull request Jul 12, 2017
…Part 1 (ifdef out)

Merge pull request #3296 from agarwal-sandeep:disablesimdjs

Put SIMDjs code in library, language and backend under #ifdef ENABLE_SIMDJS
undef ENABLE_SIMDJS
Some portion of SIMD code (IRType, OpCodesSimd, AsmJsBuiltInNames etc.) is used in WASM simd so it is left as it is. This will be take care of under #3274
Haven't ifdef'd TypeIds_SIMD* as that will make the TypeIds enum messy. During final cleanup we can clean this.

This reduce Chakra.dll x64 release binary size by 0.52 MB and ChakraCore x64 release binary size by 0.43 MB
chakrabot pushed a commit that referenced this pull request Jul 12, 2017
…kraCore - Part 1 (ifdef out)

Merge pull request #3296 from agarwal-sandeep:disablesimdjs

Put SIMDjs code in library, language and backend under #ifdef ENABLE_SIMDJS
undef ENABLE_SIMDJS
Some portion of SIMD code (IRType, OpCodesSimd, AsmJsBuiltInNames etc.) is used in WASM simd so it is left as it is. This will be take care of under #3274
Haven't ifdef'd TypeIds_SIMD* as that will make the TypeIds enum messy. During final cleanup we can clean this.

This reduce Chakra.dll x64 release binary size by 0.52 MB and ChakraCore x64 release binary size by 0.43 MB
chakrabot pushed a commit that referenced this pull request Jul 12, 2017
…code from ChakraCore - Part 1 (ifdef out)

Merge pull request #3296 from agarwal-sandeep:disablesimdjs

Put SIMDjs code in library, language and backend under #ifdef ENABLE_SIMDJS
undef ENABLE_SIMDJS
Some portion of SIMD code (IRType, OpCodesSimd, AsmJsBuiltInNames etc.) is used in WASM simd so it is left as it is. This will be take care of under #3274
Haven't ifdef'd TypeIds_SIMD* as that will make the TypeIds enum messy. During final cleanup we can clean this.

This reduce Chakra.dll x64 release binary size by 0.52 MB and ChakraCore x64 release binary size by 0.43 MB
@agarwal-sandeep agarwal-sandeep deleted the disablesimdjs branch September 19, 2017 17:22
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.

5 participants