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

SSE2 emmintrin.h #3542

Merged
merged 62 commits into from
Aug 20, 2015
Merged

SSE2 emmintrin.h #3542

merged 62 commits into from
Aug 20, 2015

Conversation

juj
Copy link
Collaborator

@juj juj commented Jun 15, 2015

This improves our SSE2 support to compile more things by expanding the emmintrin.h. Not much to review here, except instead of rewriting the SSE2 support headers, this adapts the emmintrin.h directly from Clang, which is much cleaner. I am trying to run some stuff in Nightly, but it looks like things are in flux a bit due to SIMD.js spec renamings and that has regressed, so while some things compile with this, they can't be run in SpiderMonkey atm.

@juj juj added the SIMD label Jun 15, 2015
#ifdef __EMSCRIPTEN__

// XXX TODO: Might be nice to add support for standard flags.
#define __SSE2__
Copy link
Collaborator

Choose a reason for hiding this comment

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

My instinct is that it isn't right for us to define target macros like this, even if we are emulating their APIs. We don't define __SSE__, for comparison.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that's correct, it's not intended to be defined there, but placed as temp for now, since Emscripten doesn't support -msse2 or similar command line flags. But I can remove that altogether as well.

@sunfishcode
Copy link
Collaborator

Otherwise, this patch looks like a great start!

@juj
Copy link
Collaborator Author

juj commented Jun 15, 2015

Great, thanks! Addressed the review comments.

@juj
Copy link
Collaborator Author

juj commented Jun 17, 2015

I've put up a companion pull request to this in fastcomp at emscripten-core/emscripten-fastcomp#103 , and added several changes to this pull request as well. These together enable the compilation of SSE2 code, however it does not validate as asm.js, apparently because float64x2 type is not recognized yet by OdinMonkey? (is there a bugzilla number for that?) so tests are currently run only in the default -O0 suite. With these changes, it is possible to run the test_sse1_full and test_sse2_full tests, so enabled them in the default suite, and disabled the other suites until we get validation.

@juj
Copy link
Collaborator Author

juj commented Jun 20, 2015

Updated the PR with commits that add full SSE 2 support. This pull request closes issues #2793, #2822, #2848, #2855, #2856, #3009, #3010, #3042, #3043, #3050, #3051 and #3119.

settings_changes.append('SSE2=1')
newargs.append('-D__SSE__=1')
newargs.append('-D__SSE2__=1')
newargs[i] = ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Emscripten accept -msse and -msse2 and predefine SSE and SSE2 is ok, though I am somewhat worried.

@juj juj force-pushed the sse2_emmintrin_h branch 2 times, most recently from c0aab4a to e95003e Compare August 10, 2015 16:16
@juj
Copy link
Collaborator Author

juj commented Aug 12, 2015

Ping @sunfishcode , this is now ready to be reviewed and merged from my end. Merging this requires support from emscripten-core/emscripten-fastcomp#103 first.

@juj juj force-pushed the sse2_emmintrin_h branch 3 times, most recently from 8baa375 to 257b392 Compare August 16, 2015 18:09
@juj
Copy link
Collaborator Author

juj commented Aug 18, 2015

Ping again @sunfishcode. Would you have some time to take a peek to this and the related emscripten-core/emscripten-fastcomp#103 PR?

@sunfishcode
Copy link
Collaborator

Overall this patch series looks good. My main concern now is the addition of -msse and -msse2 command-line options. We don't actually support those options in the way that x86 compilers do, so I'm not yet comfortable seeing them in emcc. Can you explain the need for these options in emcc?

@juj
Copy link
Collaborator Author

juj commented Aug 20, 2015

How do you mean we don't support these the way that x86 compilers do? If -msse is passed, we enable code targeting compilation of SSE/xmmintrin.h code, and then __SSE__ is enabled during build. If -msse2 is passed, we enable code targeting compilation of SSE2/emmintrin.h code, and __SSE2__ is enabled during the build. Since SSE2 does not validate in asm.js, we must gate depending on what is used, and since native compilers use -msse and -msse2 for this, it makes sense to reuse the same flags?

@sunfishcode
Copy link
Collaborator

I discussed this with @kripken and am now ok with it.

sunfishcode added a commit that referenced this pull request Aug 20, 2015
@sunfishcode sunfishcode merged commit d4e8da9 into emscripten-core:incoming Aug 20, 2015
@kripken
Copy link
Member

kripken commented Aug 21, 2015

There appear to be a bunch of SIMD-related failures on the bots due to this and/or the other SIMD-related pull that landed today.

@juj
Copy link
Collaborator Author

juj commented Aug 21, 2015

Pushed http://git.io/vs0f7 to resolve the SIMD test failures. On my Windows system both SpiderMonkey and node.js now go cleanly through all the SIMD tests. I'm surprised to see new NaN canonicalization related issues though.

If the bots don't get through these now, I might have to update node.js'es or SpiderMonkeys on them, I'll see about that once the dust settles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants