Support Apple Accelerate and improve MKL integration

[ChrisRackauckas]

No description provided.

[ai-maintainer]

AI-Maintainer Review for PR - Support Apple Accelerate and improve MKL integration

Title and Description ⚠️

Title is clear but description is missing

The title of the pull request is clear and indicates the purpose of the changes. However, the description is missing. It would be beneficial to include a description that provides additional context and explains the rationale for the changes.

Scope of Changes 👍

Changes are narrowly focused

The changes in this pull request are narrowly focused on adding support for Apple Accelerate and improving the integration with MKL. The modifications are concentrated in a few specific files and do not appear to be trying to resolve multiple unrelated issues simultaneously.

Documentation ⚠️

Missing docstrings for new functions and methods

Several new functions and methods have been added without docstrings. It is recommended to add docstrings to these entities to provide clear and concise descriptions of their behavior, arguments, and return values. The following entities need docstrings:

• AppleAccelerateLUFactorization
• is_new_accelerate_available
• aa_getrf!
• default_alias_A
• default_alias_b
• LinearSolve.init_cacheval
• SciMLBase.solve!

Testing ⚠️

No information about testing

The description does not provide any information about how the changes were tested. Including details about the testing methodology, such as specific test cases, frameworks, or environments used, would be beneficial to ensure that the changes have been adequately validated.

Suggested Changes

• Please add a detailed description to the pull request explaining the rationale behind the changes and any additional context that might be helpful.
• Add docstrings to the new functions and methods to describe their behavior, arguments, and return values.
• Include information about how the changes were tested. If specific test cases were used, please include them in the description.

Reviewed with AI Maintainer

[ViralBShah]

Would you ever pass ipiv to aa_getrf? If so, that probably comes in as an Int64 vector and probably should be downcast to an Int32 vector.

[ViralBShah]

I believe the smarter thing to do is to use NEWLAPACK symbols if available, falling back to the old one if not. Not sure if getrf is better in that or the same. In case the new LAPACK library has a faster LU, this is worth the effort. Some benchmarking is necessary first.

cc @vpuri3

[ChrisRackauckas]

Would you ever pass ipiv to aa_getrf? If so, that probably comes in as an Int64 vector and probably should be downcast to an Int32 vector.

We allocate all of the caches so it's safe from that. It's setup so that all caches compile up front and repeated calls then reuse the cache now, even for the info ref.

[codecov]

Codecov Report

Merging #355 (6d5aeb4) into main (464156c) will increase coverage by 47.93%.
The diff coverage is 17.74%.

@@             Coverage Diff             @@
##             main     #355       +/-   ##
===========================================
+ Coverage   25.75%   73.68%   +47.93%     
===========================================
  Files          18       19        +1     
  Lines        1254     1353       +99     
===========================================
+ Hits          323      997      +674     
+ Misses        931      356      -575     

Files Changed	Coverage Δ
src/LinearSolve.jl	90.90% <ø> (+15.90%)	⬆️
src/appleaccelerate.jl	7.27% <7.27%> (ø)
ext/LinearSolveMKLExt.jl	92.30% <100.00%> (+92.30%)	⬆️

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ChrisRackauckas]

I believe the smarter thing to do is to use NEWLAPACK symbols if available, falling back to the old one if not. Not sure if getrf is better in that or the same. In case the new LAPACK library has a faster LU, this is worth the effort. Some benchmarking is necessary first.

If I did it correctly, it doesn't seem that big of a deal in #358

[ViralBShah]

That seems correctly done. Unless they explicitly multi-threaded the getrf call, all the performance actually just comes from the matmul. openblas does have the natively multi-threaded getrf but I think it doesn't have access to the fast matmul kernels on M-series that Accelerate does: https://github.com/xianyi/OpenBLAS/blob/develop/lapack/getrf/getrf_parallel.c

I suppose the only benefit of the 64-bit version is that you don't have to convert the ipiv vector to 64-bit on return and save one small allocation.

[ChrisRackauckas]

I just cached the 32-bit version so the allocation is saved anyways. So yeah, seems like it's better to just support 32-bit there.