Skip to content

This improves our support for clang under Windows. - #817

Merged
lemire merged 20 commits into
masterfrom
dlemire/improvingsupportforclangwindows
Apr 28, 2020
Merged

This improves our support for clang under Windows.#817
lemire merged 20 commits into
masterfrom
dlemire/improvingsupportforclangwindows

Conversation

@lemire

@lemire lemire commented Apr 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #763

@lemire

lemire commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

@jkeiser This PR uses @pps83's trick of forcing the inclusion of specific headers (which one is not supposed to do) and it therefore avoids the messiness of having to do things like #define __AVX2__ 1 which is objectively worse. This approach should also be more robust.

Compared to @pps83's PR (and the current master), this PR carefully distinguishes between clang-under-windows and regular visual studio...

@lemire

lemire commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

@jkeiser I merged PR #818 into this... so we will know, some time in the next few days (joke) whether my PR fixes the CI tests.

@lemire

lemire commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

AH. Of all things, it is the new getopt thing that hits me.

@jkeiser

jkeiser commented Apr 27, 2020

Copy link
Copy Markdown
Member

@lemire is it? It looks like it has some kind of issue when it tries to set up the checkperf reference branch repository. I have a PR coming that will mitigate this ... we shouldn't be building the whole tree when we fuzz anyway.

@jkeiser

jkeiser commented Apr 27, 2020

Copy link
Copy Markdown
Member

@lemire #822 will likely get us past this (I haven't analyzed or tried to fix the underlying issue).

@jkeiser

jkeiser commented Apr 27, 2020

Copy link
Copy Markdown
Member

LOL I see, you are plagued with both problems.

@lemire
lemire marked this pull request as ready for review April 27, 2020 23:27
@lemire
lemire requested a review from jkeiser April 27, 2020 23:28
@lemire

lemire commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

@jkeiser Please review.

@lemire lemire mentioned this pull request Apr 27, 2020
Closed
jkeiser
jkeiser approved these changes Apr 27, 2020
View reviewed changes

@jkeiser jkeiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This all looks very reasonable. Just the one question, whether you can still build with cmake -DSIMDJSON_IMPLEMENTATION_HASWELL=OFF (i.e. whether westmere can compile even if we don't do all the header gymnastics in AVX2).

Comment thread include/simdjson/portability.h Outdated
Comment thread src/haswell/intrinsics.h
Comment thread src/westmere/intrinsics.h Outdated
@lemire

lemire commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

I am waiting for this to complete its run. I think that this is quite cool.

@lemire
lemire merged commit 2a1f8fa into master Apr 28, 2020
@lemire
lemire deleted the dlemire/improvingsupportforclangwindows branch April 28, 2020 02:09
@jkeiser

jkeiser commented Apr 29, 2020

Copy link
Copy Markdown
Member

This is indeed super exciting! It lets us build Windows executables with all the optimization we normally have :)

@jkeiser jkeiser added this to the 0.4 milestone May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkeiser jkeiser jkeiser approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.4

Development

Successfully merging this pull request may close these issues.

Support clang under Visual Studio

2 participants

@lemire @jkeiser