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.

Sign up for GitHub

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

Jump to bottom

Remove Simd.Js Code #4541

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Remove Simd.Js Code #4541

merged 3 commits into from
Feb 21, 2018

Conversation

Cellule
Copy link
Contributor

@Cellule Cellule commented Jan 12, 2018 •
edited
Loading

Closes #3274

This change is Reviewable

@Cellule Cellule self-assigned this Jan 12, 2018
@Cellule
Copy link
Contributor Author

Cellule commented Jan 12, 2018

FYI @arunetm

@Cellule
Copy link
Contributor Author

Cellule commented Jan 12, 2018

Mainly removed #if ENABLE_SIMDJS code, but also code related to typespec and asm.js frontend

agarwal-sandeep
agarwal-sandeep reviewed Jan 12, 2018
View reviewed changes
lib/Runtime/Language/WAsmjsUtils.cpp
@@ -289,6 +289,9 @@ template<> Types RegisterSpace::GetRegisterSpaceType<AsmJsSIMDValue>(){return WA
}
}

// The offset currently carries the total size of the funcInfo after handling the last type
funcInfo->SetTotalSizeinBytes(offset);

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This is some bug fix right? nothing to do with SIMD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to assume simd to be the last type in asm.js/wasm. With this we recalculated the size in bytes.
Without simd, it makes more sense to cache the size at the time we calculate everything and just give it back afterward, without having to know which type is last nor duplicating the math.
See AsmJsFunctionInfo::GetTotalSizeinBytes()'s implementation change for details

@agarwal-sandeep
Copy link
Collaborator 

    // double return

do we need a break here? in case Wasm::Simd::IsEnabled() is false is it ok to fall through to double?

Refers to: lib/Runtime/Language/InterpreterStackFrame.cpp:2108 in 5469106. [](commit_id = 5469106, deletion_comment = False)

@Cellule Cellule force-pushed the remove_simdjs branch 2 times, most recently from d088e1d to 1bd7096 Compare January 18, 2018 22:42
@Cellule
Copy link
Contributor Author

Cellule commented Jan 18, 2018

do we need a break here? in case Wasm::Simd::IsEnabled() is false is it ok to fall through to double?

You're right. It doesn't really make sense to have a runtime check here. We'll get bad result is wasm.simd is disabled.
I checked that we don't leak any kind of information in case we get it wrong so I just added an Assert instead. The code is cleaner now as well

@dilijev
Copy link
Contributor

dilijev commented Feb 13, 2018

Any reason someone shouldn't adopt this and get it merged?

@Cellule
Copy link
Contributor Author

Cellule commented Feb 16, 2018

Just waiting on sign off

@Krovatkin
Copy link
Collaborator

Krovatkin commented Feb 16, 2018 •
edited
Loading

@Cellule I'm very sorry about that! I completely missed it. I'll try to review it ASAP. I'm also tagging @arunetm He could also assign a back-up reviewer. FYI @Cellule I moved to a different team, so it might be a good idea to also, always assign @arunetm as a reviewer for all SIMD-related items in case I miss github notifications.

@Cellule
Copy link
Contributor Author

Cellule commented Feb 20, 2018

ping @agarwal-sandeep

agarwal-sandeep
agarwal-sandeep approved these changes Feb 20, 2018
View reviewed changes
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep left a comment

Choose a reason for hiding this comment

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

LGTM

@arunetm-zz
Copy link
Contributor

LGTM

@chakrabot chakrabot merged commit ed1bfb1 into chakra-core:master Feb 21, 2018
chakrabot pushed a commit that referenced this pull request Feb 21, 2018
@Cellule
[MERGE #4541 @Cellule] Remove Simd.Js Code
e956d46 
Merge pull request #4541 from Cellule:remove_simdjs

Closes #3274
@Cellule Cellule deleted the remove_simdjs branch February 21, 2018 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agarwal-sandeep agarwal-sandeep agarwal-sandeep approved these changes

@MikeHolman MikeHolman Awaiting requested review from MikeHolman

@Krovatkin Krovatkin Awaiting requested review from Krovatkin

Assignees

@Cellule Cellule

Labels
Codebase Quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Cellule @agarwal-sandeep @dilijev @Krovatkin @arunetm-zz @chakrabot