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 AVX/AVX2 issues with prebuilt ponyc #1663

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Conversation

SeanTAllen
Copy link
Member

We regularly get issues opened from people having problems
with the prebuilt pony packages where their platform
doesn't support AVX or AVX2 and the prebuilt packages were
built with that on.

This commit should get us building with the simplest of baselines
for users and allow them to run without issue.

The arch= idea is Sylvan's.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Mar 12, 2017
@SeanTAllen
Copy link
Member Author

I'm looking at the appveyor setup and I'm not sure what the corresponding change there is. @kulibali or @killerswan can you point me in the right direction?

What I want, pass -arch= when building if the branch is release.

@SeanTAllen
Copy link
Member Author

@ponylang/committer the other options for this would be to never run ci with AVX etc and do all builds with arch=. Thoughts?

@SeanTAllen
Copy link
Member Author

@sylvanc and I believe this should address #1661.

@SeanTAllen
Copy link
Member Author

Also, if we agree this is the correct approach, we should figure out if we need to update the homebrew formula. As no one on a mac as reported an issue for this, I'd say we don't.

@dipinhora
Copy link
Contributor

Would also address #1331.

@SeanTAllen
Copy link
Member Author

Bonus!

@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Mar 13, 2017
@SeanTAllen
Copy link
Member Author

@jemc the illegal instruction issue apparently impacts on the Docker image as well. I'm pretty ignorant on the docker front. What do we need to do to address with it?

@jemc
Copy link
Member

jemc commented Mar 13, 2017

@SeanTAllen - you should be able to add the same arch= fix to this line of the Dockerfile, where make is invoked:

RUN make \

It probably makes sense to do it in this same PR, I think.

@SeanTAllen SeanTAllen force-pushed the nicer-public-binaries branch from cb5a6fb to 5f39de4 Compare March 13, 2017 18:19
@SeanTAllen
Copy link
Member Author

@jemc pushed a change to include that

@chalcolith
Copy link
Member

For AppVeyor, the Windows build currently does not compile with either AVX or AVX2.

@SeanTAllen
Copy link
Member Author

@kulibali is that via -arch=?

@chalcolith
Copy link
Member

There's no -arch flag for the WAF script on Windows. To compile with AVX or AVX2 you can use CXXFLAGS, e.g.:

make configure CXXFLAGS=/arch:AVX
make build test

To make permanent changes you can modify the assignments to msvcDebugFlags and msvcReleaseFlags in the wscript file.

@SeanTAllen
Copy link
Member Author

@jemc so you are good with this approach and merging or do we wait til Wednesday?

@killerswan
Copy link
Member

killerswan commented Mar 13, 2017

@you'll also want to modify the README (line ~385) section about AVX, and perhaps the section below it.

Are there any other flags in native (see here and here) which we'll want to re-add manually?

@jemc
Copy link
Member

jemc commented Mar 13, 2017

@SeanTAllen I'm fine with merging before Wednesday, if all your questions are resolved before then.

However, seems like @killerswan now has some outstanding concerns to resolve.

@SeanTAllen
Copy link
Member Author

@killerswan i'd say this is a good place to start from and we could add things back in. in general, i think it's reasonable to say "if you want best performance, build from source".

We regularly get issues opened from people having problems
with the prebuilt pony packages where their platform
doesn't support AVX or AVX2 and the prebuilt packages were
built with that on.

This commit should get us building with the simplest of baselines
for users and allow them to run without issue.

The `arch=` idea is Sylvan's.
@SeanTAllen SeanTAllen force-pushed the nicer-public-binaries branch from 5f39de4 to 931d512 Compare March 13, 2017 23:59
@SeanTAllen
Copy link
Member Author

SeanTAllen commented Mar 13, 2017

@killerswan README updated. Nice catch. Thanks.

@SeanTAllen
Copy link
Member Author

If this passes, I'm going to merge and do a release so we can take care of the issues that people are currently hitting and we can always revisit the...

Are there any other flags in native (see here and here) which we'll want to re-add manually?

issue that @killerswan raised.

@SeanTAllen SeanTAllen added needs discussion during sync and removed do not merge This PR should not be merged at this time needs discussion during sync labels Mar 14, 2017
@SeanTAllen
Copy link
Member Author

I'm leaving on the "needs discussion during sync" so we can discuss the additional issues re: native and "should we change OSX" and "should we use -arch= for all CI builds not just release`

@SeanTAllen SeanTAllen merged commit 66db11d into master Mar 14, 2017
@SeanTAllen SeanTAllen deleted the nicer-public-binaries branch March 14, 2017 02:43
ponylang-main added a commit that referenced this pull request Mar 14, 2017
@SeanTAllen
Copy link
Member Author

People are still reporting this as a problem. Apparently either

  1. i implemented this wrong
  2. sylvan's idea fix the problem

@killerswan
Copy link
Member

In the diff I notice that one place you call make arch=, and in another make -arch=...

@SeanTAllen
Copy link
Member Author

sadly its the docker on that is incorrect.

@SeanTAllen
Copy link
Member Author

so i did it correctly for what i changed in travis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants