Repository navigation
Support Apple's new Accelerate interface (closes #43) - #88
Merged
Merged
Conversation
The LAPACK++ counterpart to blaspp PR icl-utk-edu#74, requested in issue icl-utk-edu#43 and never started. Under ACCELERATE_NEW_LAPACK, lapack/fortran.h stops declaring its own prototypes and includes Apple's Accelerate header, whose declarations carry the asm-label decoration binding $NEWLAPACK (and $NEWLAPACK$ILP64) symbols. The LAPACK_xxx macro layer is unchanged, so all call sites compile as-is; a self-declared undecorated prototype would silently bind the legacy LAPACK 3.2.1 symbols instead (the failure mode observed in issue icl-utk-edu#43). All 1,295 prototypes sit behind a new LAPACK_PROTO guard; with the feature off, the preprocessed header is byte-identical to before. Details: - lapack_int becomes long under ILP64 + new Accelerate, matching Apple's __LAPACK_int (int64_t is long long on LP64 Darwin -- a distinct type); lapackpp_float_return stays double only for the OLD f2c-convention Accelerate. - The Schur-form callback typedefs (LAPACK_S_SELECT2 & co.) alias Apple's __LAPACK_*_func_ptr types under the new interface, and every gees/gges-family call site casts its callback through them (the complex paths already did; the real/double paths now match). - Config probes (lapack_potrf, lapack_pstrf, lapack_version, and the shared config.h) include Apple's header under ACCELERATE_NEW_LAPACK so they bind decorated symbols; ilaver then reports 3.9.1 instead of the legacy 3.2.1. Neither old nor new Accelerate passes hidden string-length arguments. configure.py gains the new/old Accelerate probe choices. Verified on Linux (MKL): preprocessed fortran.h identical with the feature off; full build clean against a blaspp carrying PR icl-utk-edu#74; the version probe reports MKL's LAPACK correctly. First macOS execution is the fork's CI.
Apple's new-Accelerate lapack.h omits the const qualifier on some
read-only pointer arguments (the orm* multiply-by-Q family's A, opmtr's
AP, bdsvdx's D and E). The complex-variant call sites already cast
through their type conversion; the real-variant call sites passed the
wrapper's const parameters directly and fail to compile against Apple's
prototypes ('would lose const qualifier'). Cast at the 34 affected call
sites, found by comparing all 1,295 declarations against Apple's header
position by position rather than iterating CI rounds.
Same fix as the blaspp branch, where this file was copied from: the merge resolution lost the 'if (test_accelerate):' header (attaching the Accelerate block to the BLIS section) and left a stale duplicate of the choices referencing undefined variables; blas=blis make builds crashed with NameError. Guard restored, duplicate dropped; verified by driving configure.py's choice-building with blas=blis and blas=apple.
…ate choices The previous commit copied BLAS++'s config/lapack.py wholesale, but the two projects' config modules differ: BLAS++'s exports print_value and font, LAPACK++'s does not, so every make-path build died at import with 'cannot import name print_value from config'. Restore LAPACK++'s file and apply only the four new/old Accelerate choices to it, leaving the imports untouched. Verified by importing the module and running configure.py through both the blas=apple and blas=blis paths.
This was referenced Aug 10, 2026
mmelnich
added a commit
to BallisticLA/RandLAPACK
that referenced
this pull request
Aug 12, 2026
…ark's BLAS++/LAPACK++ review) (#157) > [!WARNING] > **This is a temporary change and is meant to be reverted.** > It is one clean `git revert` of a single commit. ### Revert when these land The quarantine exists only until Apple's new Accelerate interface is in use. Both are open and awaiting review: | PR | Status | |---|---| | [icl-utk-edu/blaspp#134](icl-utk-edu/blaspp#134) — New Apple Accelerate support (#74, rebased and completed) | open | | [icl-utk-edu/lapackpp#88](icl-utk-edu/lapackpp#88) — Support Apple's new Accelerate interface (closes #43) | open | **Order matters:** lapackpp#88 depends on the `defines.h` from blaspp#134, so #134 merges first. blaspp#134 is a rebased and completed continuation of the stalled [blaspp#74](icl-utk-edu/blaspp#74). Once both are merged, [RandLAPACK#155](#155) (retire legacy-Accelerate accommodations) becomes mergeable and `TestQB.Polynomial_Decay_general1` should pass on its own — at which point the warning added here fires and tells you to delete this quarantine. ## Why `TestQB.Polynomial_Decay_general1` fails on Apple Silicon because Apple's old Accelerate LAPACK has a broken divide-and-conquer `gesdd`. It has been left failing deliberately, as a canary for the planned migration. The side effect is that **`core-macos` has been permanently red** — `build` and `build-asan`, on `main` and on every branch. A lane that is always red carries no information: you cannot distinguish a new macOS regression from the known one, so the job gets ignored and the canary signals to nobody. ## What this does Quarantines the test without discarding the signal: - The main `ctest` run excludes it, so the job reflects real macOS health. - The test is then **still run**, separately, and cannot fail the job. - Every macOS run prints a warning that the suppression is active and names the condition for removing it. - If the test ever **passes**, the job prints a louder warning saying to delete the quarantine. That last point matters: the migration signal is preserved and actually improved. Previously it was "a red job turns green", which nobody was watching because the job was always red. Now it is an explicit annotation telling you what to delete. ## Scope Deliberately surgical — `core-macos.yaml` (both jobs) and the corresponding `docs/CI.md` entry, nothing else, so the revert stays clean. The dead `TestABRIK.ABRIK_catch_instability` exclusion in these same lines is left alone for that reason.
mgates3
requested changes
Aug 22, 2026
mgates3
left a comment
mgates3
left a comment
Collaborator
There was a problem hiding this comment.
A few cleanup items, but generally looks good. Thanks for tackling this.
bdsvdx: D and E are written by the routine (elements below an internal threshold are zeroed; see Reference-LAPACK/lapack#1362), so they are not const. Remove const from the wrappers, wrappers.hh, and our fortran.h prototypes (Apple's prototypes already agree), revert the casts, and mark D and E [in,out] in the docs. opmtr: AP is modified and restored by the routine, so it is not const by the C++ definition. Same treatment; the real upmtr aliases forward AP and change with it. The complex upmtr wrappers keep their pre-existing casts (out of scope here). fortran.h: indent the Accelerate callback-typedef block inside its #if/#else/#endif. config: remove the #pragma message debugging and point the stdlib.h workaround comments at the explanation in fortran.h, matching the equivalent cleanup in the BLAS++ PR. Verified on Linux/oneMKL against blaspp@new-accelerate: library and tester build clean; upmtr tester passes.
Contributor
Author
|
@mgates3 Everything should be addressed now. |
mgates3
approved these changes
Aug 27, 2026
mmelnich
added a commit
to BallisticLA/RandLAPACK
that referenced
this pull request
Aug 27, 2026
…155) ## ✅ Upstream requirement met (2026-08-27) The gate this PR was waiting on has cleared: icl-utk-edu merged the new-Accelerate support in both BLAS++ ([icl-utk-edu/blaspp#134](icl-utk-edu/blaspp#134)) and LAPACK++ ([icl-utk-edu/lapackpp#88](icl-utk-edu/lapackpp#88)) on 2026-08-27. core-macos builds its dependencies from the icl-utk-edu masters, so it now exercises the new interface for real. **Refreshed 2026-08-27**, same scope plus: - `main` merged in (the branch was 6 commits behind). - The `TestQB.Polynomial_Decay_general1` quarantine from #157 is reverted here, per #159: this PR is what makes the test pass, so the two must land together. Closes #159. - Accidentally committed `build/` artifacts removed from the branch, and a top-level `build/` entry added to `.gitignore` so they cannot recur. - The core-macos dependency-cache key gained an `accelerate` tag, because earlier caches hold a BLAS++ built without `-Dblas=accelerate` and a cache hit would skip the new-interface guard. - Linux verification against the refreshed branch: 772/772 tests pass (Release, oneMKL). ## What this PR does Makes macOS a full-library, first-class platform by removing all 18 `__APPLE__` guards, and switches macOS CI to Apple's new Accelerate interface. **Background.** Apple ships two LAPACKs in Accelerate. The legacy default is 3.2.1 (2009): `gesdd` returns wrong results on Apple Silicon (the reason core-macos has carried a deliberately red canary test), and routines RandLAPACK needs (`orhr_col`, `geqrt`, ...) don't exist — the reason BQRRP threw "not supported" on every Mac and ten benchmarks were stubbed out. The new interface (macOS ≥ 13.3, `ACCELERATE_NEW_LAPACK`, LAPACK **3.12** on current SDKs) fixes both, verified on this repo's CI: [run 31065130007](https://github.com/BallisticLA/RandLAPACK/actions/runs/31065130007) — 311/311 tests pass on macOS, including the gesdd canary, all of TestBQRRP (first macOS execution ever), and `test_orhr_col`. **Changes:** - **Drivers**: BQRRP's runtime "not supported on Apple" throw removed; HQRRP's CholQR-path stub removed; CQRRPT's BQRRP-subroutine path enabled on macOS. - **Tests**: `test_bqrrp` (whole file), the guarded sections of `test_cqrrpt`/`test_hqrrp`/`test_bqrrp_gpu`, and `test_orhr_col` now compile everywhere — macOS gains 16 tests. - **Benchmarks**: the ten "cannot run on Apple machines" stub mains removed; real benchmark code everywhere. - **CI**: core-macos builds BLAS++ with `-Dblas=accelerate` and hard-fails if the new interface wasn't selected (no silent legacy fallback). The canary test is included and expected green — the canary era ends with this PR. - **Docs**: INSTALL.md documents the macOS requirement (new interface or Homebrew OpenBLAS; `install.sh` on macOS uses OpenBLAS and is unaffected). docs/CI.md replaces the quarantine entry with the new-Accelerate arrangement. **Follow-up, not in this PR:** advancing the pinned BLAS++/LAPACK++ commits in `install/install.sh` and the Windows CI provisioner, allowing Accelerate ILP64, and re-evaluating the macOS installer default are tracked in #165 (RandLAPACK) and BallisticLA/RandBLAS#189.
This was referenced Aug 27, 2026
Merged
mmelnich
added a commit
to BallisticLA/RandLAPACK
that referenced
this pull request
Aug 27, 2026
…#172) ## Summary Follow-up to #155, which retired the legacy-Accelerate accommodations in the library and CI. This PR does the installer half: with new-Accelerate support merged upstream on 2026-08-27 ([icl-utk-edu/blaspp#134](icl-utk-edu/blaspp#134), [icl-utk-edu/lapackpp#88](icl-utk-edu/lapackpp#88)), the reasons macOS defaulted to Homebrew OpenBLAS — the legacy `gesdd` bug, the missing routines, the missing ILP64 — are gone. The default macOS build now uses Accelerate through Apple's new interface and needs no Homebrew BLAS. (ILP64 turned out to be blocked upstream — see below.) Closes #165 except its ILP64 item, which is split into #173: this PR's own install-macos lane discovered that LAPACK++ does not compile against Accelerate ILP64 (`lapack_int` becomes Apple's `long` while the wrappers assume `int64_t`; filed upstream as icl-utk-edu/lapackpp#89). Accelerate is therefore constrained to LP64 for now — the same width the old OpenBLAS default produced on macOS, so nothing regresses. RandBLAS mirror: BallisticLA/RandBLAS#198 (unaffected: RandBLAS does not need LAPACK++, and its Accelerate ILP64 lanes are green). ## Changes - **Pins**: `installers/install.sh` and `.github/actions/setup-randlapack-deps-windows/setup.ps1` advance BLAS++ to `2d8d4e9` (the #134 merge) and LAPACK++ to `b9439cf` (the #88 merge). - **Backend default**: `--blas=auto` on macOS resolves to `accelerate` (previously `openblas`). `--blas=openblas` remains fully supported. - **Correctness guard**: a build in which BLAS++ silently fell back to Apple's LEGACY interface is refused immediately after the BLAS++ build, by checking the generated `defines.h` for `ACCELERATE_NEW_LAPACK` — the legacy `gesdd` computes wrong singular values, so this must be a hard error, not a warning. The numerical `gesdd` conftest stays as the second net. On macOS < 13.3 the error message points at `--blas=openblas`. - **Width**: the gesdd warning is removed. Accelerate stays LP64 for now, and an explicit `--blas-int=ilp64` is refused with a message citing icl-utk-edu/lapackpp#89 / #173 (previously the refusal cited the long-gone interface gap). - **Portability fix found by this PR's CI**: macOS ships bash 3.2, where expanding an empty array under `set -u` is an "unbound variable" error; with the old OpenBLAS default the affected arrays were never empty on macOS, so the default flip exposed it (it was already reachable with an explicit `--blas=accelerate` or `--blas=mkl` on any Mac). All possibly-empty array expansions now use the bash-3.2-safe guard, and the install-script lanes print `install.log` on failure so errors of this class are never silent again. - **install-script CI (macOS)**: the default lane deliberately does NOT `brew install openblas` anymore, and asserts the summary reports `accelerate, LP64`; a new explicit `--blas=openblas` leg (with its own project dir and test-suite run) keeps that route exercised. - **Rename, mirroring the review on BallisticLA/RandBLAS#198**: the `install/` folder is now `installers/`. The root `install.sh` wrapper keeps working (`bash install.sh` unchanged for users) and now execs `installers/install.sh`; references updated in INSTALL.md, INSTALL_SCRIPT.md, INSTALL_WINDOWS.md, docs/CI.md, the install-script workflow, and the Windows guard scripts. - **Docs**: INSTALL.md, INSTALL_SCRIPT.md (§6.1 table, §6.2 width table, §6.4 rewritten), docs/CI.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #43 — the LAPACK++ half of new-Accelerate support, in the shape described there: Apple's decorated prototypes, LAPACK++'s own macro layer. Companion to the BLAS++ PR from
BallisticLA:new-accelerate(the #74 revival); builds on the defines that finder exports.Context. Accelerate's default LAPACK is 3.2.1 (2009). Since macOS 13.3,
ACCELERATE_NEW_LAPACKexposes a modern implementation (ilaverreports 3.12 on current SDKs) whose$NEWLAPACKsymbols are reachable only through Apple's asm-label-decorated prototypes — a self-declaredilaver_binds the legacy symbol, which is exactly the garbage-version failure mode from the #43 thread.What this does.
include/lapack/fortran.h: underACCELERATE_NEW_LAPACK, includes<Accelerate/Accelerate.h>instead of declaring its own prototypes; all 1,295 prototypes now sit behind aLAPACK_PROTOguard, and theLAPACK_xxxmacro layer is untouched, so call sites compile unchanged. With the feature off, the preprocessed header is byte-identical to before. Apple passes no hidden strlen arguments, soLAPACK_FORTRAN_STRLEN_ENDstays undefined there.include/lapack/config.h:lapack_int=longunder ILP64 + new Accelerate (Apple's__LAPACK_int;int64_tislong longon Darwin);lapackpp_float_returnstaysdoubleonly for the legacy f2c-convention Accelerate.LAPACK_S_SELECT2& co. alias Apple's__LAPACK_*_func_ptrtypes under the new interface; the real-path call sites ingees/geesx/gges/gges3/ggesxgain the cast the complex paths already had.conston read-only arguments (theorm*family'sA,opmtr'sAP,bdsvdx'sD/E), the 34 affected call sites cast — found by comparing all 1,295 declarations against Apple's header position by position.lapack_potrf,lapack_pstrf, the sharedconfig.h,configure.pychoices) get the same treatment as their BLAS++ siblings in rocm: fix __HIP_PLATFORM_AMD__ #74 — plusconfig/lapack_version.cc, whose self-declared prototype was the source of the wrong version reports.Coverage. Only 50 of the 1,295 wrapped routines are absent from Apple's LAPACK — all already behind the existing
LAPACK_HAVE_XBLAS/LAPACK_HAVE_MATGENguards, whose probes correctly fail on Accelerate. The newest version-gated wrappers (orhr_col/unhr_col, 3.9.0) clear Accelerate's reported version. Nothing needs stubbing.Validation.
macos-latest): full build against the new interface; a downstream consumer (RandLAPACK) passed its complete suite on top, includinggesdd-based tests that fail on legacy Accelerate andorhr_col-based paths the legacy interface lacks: https://github.com/BallisticLA/RandLAPACK/actions/runs/31065130007