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

Enable updated Accelerate headers for Darin kernel >= 22.4 #8099

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

PaulWessel
Copy link
Member

Getting tired of the tantrum Xcode has added if we call the deprecated dgemm_ without including the new headers via compiler flag -DACCELERATE_NEW_LAPACK. This PR has ben following changes:

  1. cmake/ConfigUserAdvancedTemplate.cmake: Adds -DACCELERATE_NEW_LAPACK to the compiler flags if we are on Darwin >= 22.4
  2. CMakeLists.txt: Determine if we are on version 22.4 and if so set Cmake variable NEW_ACCELERATE_LAPACK to 1 (else 0).
    3. gmt_vector.c: Use #ifdef ACCELERATE_NEW_LAPACK to cast our parameters using Apple's new dgemm_ definition.

This PR now compiles quietly for me on macOS Sonoma. Ideally:

  • Someone who knows Cmake better than me (@seisman, @remkos ?) should see if the new code in the advanced template can be simplified.
  • Give it a try if you have an older kernel since I am unable to test that.
  • Also build on non-macOS to ensure nothing funny happened.

Getting tired of the tantrum Xcode has added if we call the deprecated dgemm_ without including the new headers via -DACCELERATE_NEW_LAPACK. This PR has ben following changes:

cmake/ConfigUserAdvancedTemplate.cmake: Adds  -DACCELERATE_NEW_LAPACK to the compiler flags if we are on Darwin >= 22.4
CMakeLists.txt: Determine if we are on >= 22.4 and if so set NEW_ACCELERATE_LAPACK to 1 (elase 0).
gmt_vector.c: Use #ifdef ACCELERATE_NEW_LAPACK to cast our parameters using Apple's new definitions.

Now complies quietly for me on macOS Sonoma.
@PaulWessel PaulWessel added the maintenance Boring but important stuff for the core devs label Nov 28, 2023
@PaulWessel PaulWessel added this to the 6.5.0 milestone Nov 28, 2023
@PaulWessel PaulWessel requested a review from a team November 28, 2023 17:20
@PaulWessel PaulWessel self-assigned this Nov 28, 2023
Copy link
Contributor

@remkos remkos left a comment

Choose a reason for hiding this comment

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

As you have notices, I needed to make some changes:

  • Renaming NEW_ACCELERATE_LAPACK to ACCELERATE_NEW_LAPACK did not have the result I expected, though I should have realised that. Nonetheless, consistency is practical.
  • Putting stuff in ConfigUserAdvancedTemplate.cmake did not make much sense. I would not have looked into the template myself. It should be part of the CMakeList hierarchy.
  • The whole stuff needed to be moved to src/CMakefile.txt to have effect, because only there it is determined if LAPACK is even there.

@remkos
Copy link
Contributor

remkos commented Nov 28, 2023

It works now for me.
@PaulWessel and @seisman please check if this works for you.
It should probably work "out of the box" with a simple make. Otherwise redo a cmake in your build directory, then make.

@PaulWessel
Copy link
Member Author

I guess I copied the new stuff from that template to my own advanced cmakefile and as far as I can tell it found everything and compiled as expected. So I think it works IF you update that file.

I agree that users on recent macs who already have GMT via git will not notice the change since they may have an advanced file that is not getting updated. I can look at this tomorrow pm unless you do something first.

@remkos
Copy link
Contributor

remkos commented Nov 28, 2023

It works now for me without having touched my ConfigUser.cmake, which is what I wanted to achieve, and as you pointed out is what users would likely expect. We do not need to bother them with switching logic in their user configuration.

I have nothig to change. Just wait for @seisman and you to see if it works for you.

@PaulWessel
Copy link
Member Author

Great, works fine for me - yours is the right solution. All tests still pass that passed before.

@PaulWessel PaulWessel changed the title WIP Enable updated Accelerate headers for Darin kernel >= 22.4 Enable updated Accelerate headers for Darin kernel >= 22.4 Nov 29, 2023
@PaulWessel PaulWessel merged commit 7693dae into master Nov 29, 2023
6 checks passed
@PaulWessel PaulWessel deleted the update-macos-lapack branch November 29, 2023 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants