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

Disable neon for armhf if not supported by C/C++ compiler #2672

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

ikavalio
Copy link
Contributor

Hi, this minor PR is a workaround for llvm's bug 30842. The issue is that neon instructions are enabled by default for armhf even if target CPU doesn't support them. As a result llvm backend may generate code that has unsupported instructions and program can crash with SIGILL.

It's not a big issue for ponyc as neon can be disabled with ponyc --features=-neon, but it's problematic for libponyc unit tests as compilation target is configured automatically and neon can't be disabled easily for such cases. As a result unit tests are failing on tests like BoxedTupleIsBoxedTuple where vectorisation is used which isn't great.

My current solution is to disable neon by default (setting --features= for ponyc explicitly will revert the behavior) if neon instruction set was disabled for C/C++ compiler. I created a new var in Makefile fpu that corresponds to -mfpu flag and can be redefined for arm targets, otherwise it doesn't have any effect.

I hope it makes sense.

Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

It makes sense to me, but I'd like someone else to take a look as well.

@SeanTAllen
Copy link
Member

We discussed at sync and no one felt knowledgeable enough to merge. @sylvanc or @Praetonus perhaps.

@SeanTAllen SeanTAllen merged commit c0cb529 into ponylang:master Jun 20, 2018
@sylvanc
Copy link
Contributor

sylvanc commented Jun 20, 2018

This looks good! Thank you @ikavalio

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.

4 participants