-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
Cleanup handling of march=native #1544
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #1544 +/- ##
===========================================
+ Coverage 83.87% 83.90% +0.02%
===========================================
Files 132 132
Lines 10843 10843
Branches 2801 2801
===========================================
+ Hits 9095 9098 +3
+ Misses 1049 1046 -3
Partials 699 699 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions whether check_neon_compiler_flag
and check_acle_compiler_flag
still need to be run if native
. I can't really tell.. Otherwise looks gtm.
Rebased. |
I think the correct thing to do is to rewrite those checks to instead use the native flag, and then verify that the neon/acle/etc code still compiles with native. And let that decide whether we attempt to include those optimized functions in the library or not. Arm "native" was completely broken in configure before this PR, and with this it is less broken. I am tempted to just rip out the native support from configure completely, for all arches, rather than attempt to fix it and maintain it, and only support such advanced usage in CMake. It is a niche feature, and it is not trivial to get right with all the arch-specific code we have. |
Regarding my question, I don't really expect it to be resolved with this PR. It is just a strange scenario. |
Rebased and removed commit making changes to Configure. CMake changes remain unchanged. |
Simplify handling of native in CMake.
As a side note, ARM support in configure is quite messy. There is a lot of feature tests being defined directly in the final "Set arch specific flags" part (line ~1500+). All the other arches have their test functions defined well before that, and only have function calls in that part. Better readability and easier to reuse the test code.