Skip to content

Retire legacy-Accelerate accommodations: macOS runs the full library - #155

Draft
mmelnich wants to merge 1 commit into
mainfrom
retire-legacy-accelerate
Draft

Retire legacy-Accelerate accommodations: macOS runs the full library#155
mmelnich wants to merge 1 commit into
mainfrom
retire-legacy-accelerate

Conversation

@mmelnich

@mmelnich mmelnich commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

⛔ Do not merge yet

This PR must stay open until icl-utk-edu merges the new-Accelerate support in BLAS++ and LAPACK++ (the PRs from BallisticLA:new-accelerate on both repos). It removes every legacy-Accelerate accommodation from RandLAPACK, so a macOS build only works against a BLAS++/LAPACK++ that can select Apple's new interface. The core-macos job here deliberately builds from the icl-utk-edu masters: it stays red until upstream merges, and flips green by itself when they do — that flip is the merge signal.

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 — 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).

Merge order: icl-utk-edu/blaspp PR → icl-utk-edu/lapackpp PR → core-macos here turns green → merge this.

Apple's legacy Accelerate LAPACK (3.2.1, 2009) breaks gesdd on Apple
Silicon and lacks orhr_col/geqrt & co., which forced 18 __APPLE__
guards into the codebase: BQRRP threw 'not supported' on every Mac,
HQRRP's CholQR path was stubbed, CQRRPT could not delegate to BQRRP,
test_bqrrp and test_orhr_col were compiled out, and ten benchmarks were
stub mains. Apple's new interface (ACCELERATE_NEW_LAPACK, macOS >= 13.3,
LAPACK 3.12 on current SDKs) fixes all of it -- verified end to end:
311/311 tests pass on macOS including the gesdd canary, all of
TestBQRRP, and test_orhr_col.

Remove every guard; switch core-macos to the new interface
(-Dblas=accelerate + a hard defines.h check so a silent fallback to the
legacy interface fails the job); document the macOS requirement in
INSTALL.md. Requires BLAS++/LAPACK++ with new-Accelerate support, so
this stays unmerged until icl-utk-edu merges the two PRs from
BallisticLA:new-accelerate -- core-macos here is deliberately red until
then and flips green by itself when upstream lands.
mmelnich added a commit 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.
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.

1 participant