Repository navigation
Conversation
A third vendor arm, `--vendor aocl` / feature `link-aocl`, that runs the existing LAPACK row body unchanged: the same prepared buffers, the same LAPACKE in-place entry points (`LAPACKE_dgetrf`, `LAPACKE_dpotrf`, ...), the same `LAPACKE_set_nancheck(0)` before anything is measured, the same check outside the clock. Only the library selection differs: * build.rs links `-lflame -lblis-mt -laoclutils -lm` and GNU OpenMP by its soname from the versioned `AOCL_ROOT`; no run-time rpath, the loader path is `LD_LIBRARY_PATH=$AOCL_ROOT/lib` in the command. Two `link-*` features at once are refused. * vendor.rs sets the budget the way the MKL arm does (`omp_set_num_threads` on the thread that times the row) and reads back OpenMP, BLIS and libflame; a stray `BLIS_NUM_THREADS`/`FLA_NUM_THREADS` that overrides one of them is refused instead of silently changing the row. The identity records both libraries' version strings, the files actually mapped, and the dispatched kernels (`blis=zen5, flame=avx512` on Zen 5). * build_for_faer_rev.sh resolves `/opt/AMD/aocl/*/gcc`, requires libflame and aocl-utils, and also drops BLIS_NUM_THREADS/FLA_NUM_THREADS. AOCL defines the same symbols as MKL and OpenBLAS, so it is its own binary, as AGENTS.md requires of every arm. record_run.py / validate_run.py are not taught the new vendor in this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #52: a vendor can report more than one thread count, and AOCL does - OpenMP's, BLIS's and libflame's, each per calling thread, the last two overridable by BLIS_NUM_THREADS / FLA_NUM_THREADS. The AOCL arm folded them into one number, so OpenMP 1 / BLIS 4 / libflame 1 under a 4-thread budget would have passed the check and been recorded as 4. * vendor.rs: each arm reports its counts by knob (`thread_counts`); `require_threads` aborts unless every one equals the budget and names all of them; `reported_threads` (the CSV's vendor_threads) is the common value only when they agree; `fbench info` prints `threads.vendor_detail=omp:N blis:N flame:N` (`mkl:N`, `openblas:N`). * main.rs: the same compare-and-abort runs again right after each row's timed loop, so a budget that moved during priming or timing is refused rather than recorded. Order per row: configure_threads (main.rs:517, setter :423, read-back :426) -> require_threads (:579) -> measure_row (:613): prime and timed loop (timing.rs:25-36) -> require_threads again (:617). * README: what reproducing an AOCL number needs (AOCL_ROOT, run-time LD_LIBRARY_PATH with no rpath by design, AOCL kept out of ldconfig). CSV schema unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the faster of MKL and AOCL Two vendor arms can be measured on one AMD host, and neither dominates: from the 2026-11-07 session on EVO-X2 (n=1024, f64, reps 5, three process sets), AOCL is the faster vendor row in 12 of the 24 cells both arms carry (gemm in both orders at every thread count, f-order cholesky at every thread count, c-order cholesky at 1T, f-order qr at 1T and 8T), and MKL is faster in the rest, by up to 6x on c-order lu. A page that names one of them as "the vendor" understates the other by that factor. So: * fbench/src/vendor.rs: the AOCL arm gets the host requirement it always had in fact -- it is an x86-64 package whose libflame and threaded BLIS dispatch to AMD's kernels. `host_cpu_vendor()` reads `/proc/cpuinfo`'s `vendor_id` (macOS: `sysctl -n machdep.cpu.vendor`) and does not default: a host that cannot be read is not AMD. `amd_host_refusal(Result<String, String>)` is the rule, split from the probe so a test can state it for a host it is not running on, and `host_refusal()` is `None` for every other arm (MKL and OpenBLAS run wherever they load). * fbench/src/main.rs: the row loop asks before it touches any thread budget, and writes the row as `skipped` with the reason in the note -- `aocl arm requires an AuthenticAMD host (this host: GenuineIntel)` -- for every cell the run asked for. Not a hard failure, and not a missing row: the same contract as a family the vendor library has no route for. * scripts/gen_comparison.py: `--aocl <run dir>` joins a second vendor arm and adopts, per cell, the faster of the MKL and AOCL rows. Nothing in the code lists which cells AOCL wins -- the choice is read off the two recordings, so a re-measured cell moves it -- and a `reference` column names the library each cell's ratio uses, with both measured values still in the table. A cell the AOCL arm carries no row for falls back to MKL and is marked `MKL*`, with the arm's own notes (its `skipped` rows) listed under the table. Runs of different hosts are refused: the host identity (`hostname`, `cpu`, `arch`, `logical_cpus`) comes from each `run.yaml` and must match, because "the faster of two rows" is only a statement about one machine. `--openblas` becomes optional; without `--aocl` the page is the one this script has always written. The OpenBLAS summary also changes: its pair count and median were computed from `r[6]`, which is the upstream/fork ratio, not the OpenBLAS one, so the line read "1356 against lapack-openblas" and "OpenBLAS 1.956" where 972 cells carry an OpenBLAS row and their median is 1.230. * docs/vendor-reference.md, README.md: what the reference is and why it is per cell, why AOCL is AMD-only, what `MKL*` means, and what the page still does not claim. * tests/test_comparison_aocl.py: six tests over manifest-plus-CSV fixtures -- AOCL chosen where it measured faster, MKL chosen where it measured faster, fallback with the arm's own reason, two hosts refused, an all-skipped AOCL arm still rendering (the non-AMD case as the page sees it), and the two-arm page unchanged. The harness side of the non-AMD case is two unit tests in `fbench/src/main.rs` / `vendor.rs`. No numbers, thresholds or protocol were changed; the CSV schema is unchanged.
249c867 to
a97bdc2
Compare
GPT-6.1-sol 敵対的レビュー（
|
…ed row precedes the budget
Two defects from the review of the AOCL arm (faer-benchmark#54).
1. `scripts/gen_comparison.py` took each cell's value from the last `run*.csv`
the glob read. A cell measured in three process sets was therefore the third
set's number, not the cell's: AOCL at 5/5/20 ms against MKL at 10 ms made the
reference MKL at 10 ms instead of AOCL at its median 5 ms, and the page showed
20 ms as AOCL's time. Every arm and every row went through that one function
(`load`), so MKL and OpenBLAS rows carried the same defect; `control` already
took the median per cell, which is what made the two disagree.
`read_sets` is now the one parser: it keeps the per-set values (which
`control` needs for the between-start range) and the skipped-row notes, and
`load` reduces a cell to the median over its sets. It also reads the files the
manifest names (`result_files`) instead of every `run*.csv` in the directory,
which is what the validator and the recorded pages read: a leftover copy of a
slow set is now ignored instead of moving the median (5/5/20 plus a copy of the
20 is 12.5, which would hand a cell to MKL at 10 while nothing was re-measured).
Three tests, each failing on the parent: 5/5/20 ms of AOCL against MKL's 10 ms
chooses AOCL at 5; a median that ties MKL (5/5/20 against 5) still stays with
MKL, with AOCL's column asserted to be 5 so the tie is not decided by the
wrong number; and a CSV the manifest does not name is ignored.
2. The host rule was read per row, but `configure_threads` had already run: on a
non-AMD host with `BLIS_NUM_THREADS=2 FLA_NUM_THREADS=2`, `fbench-aocl run
--threads 1 --rows lapack-aocl` exited 2 in the vendor read-back ("aocl
reports omp:1 blis:2 flame:2, but this row declares 1 threads") before it
could write the `skipped` row the arm promises -- and the recorder stops the
recording on a non-zero exit, so the arm was lost rather than refused.
The rows are now chosen before the budget, and `vendor_budget_required` says
whether the vendor library's budget is a precondition at all: it is not when
the run asks for the vendor row and this host refuses it, because that row
never calls the library. `vendor::set_threads` no longer reads the budget back
itself (the caller does it once, through `require_budget`), which is what let
the read-back be conditional; every other run keeps it, `--rows faer-fork`
included, so a row labelled `1T` still means the library also ran on one
thread. The harness's own Rayon pool is always configured and checked, so the
CSV's `rayon_threads` stays truthful.
`fbench/tests/host_refusal.rs` runs the reported command against the real
binary with those two variables set and asserts exit 0 with two `skipped` rows
naming the host; it says so out loud where it returns early (a non-AOCL build,
or an AMD host, where the row is measured instead of refused). A unit test
pins the rule for a host it is not running on.
Verified on EVO-X2 (AMD Ryzen AI MAX+ 395, AOCL 5.2.0, fork t4a-faer 2b87010c,
upstream faer-v0.24.4). With a `GenuineIntel` `/proc/cpuinfo` bind-mounted into a
mount namespace: the pre-fix binary exits 2 and writes no CSV, this one exits 0
with two `skipped` rows (`rayon_threads=1`, `vendor_threads` empty because the
three library counts disagree), and the integration test fails on the parent's
source and passes here. On the real AMD host both binaries measure the same six
`ok` rows, and the vendor read-back still refuses a mismatched budget when the
vendor row *is* measured. 12 unit tests and 1 integration test pass on that
build; `python3 -m unittest discover -s tests` passes 24 tests.
Not changed: with a thread environment that disagrees with `--threads`, a refused
row's `vendor_threads` is empty and the pinned toolkit's v2 evidence check
(`timing_evidence.validate`) rejects such a recording. That is the environment
being wrong rather than the host, and the recorder's scrubbed environment gives
`vendor_threads=1` on the same refused rows (measured); the harness does not
invent a count for a library it never called. Noted in docs/vendor-reference.md.
Review fixes (8557f0b)
The two blocking findings from the review of the previous head: 1. The reference was decided by the last CSV.
2. The host refusal came after the thread-budget check. Measured on EVO-X2 (Ryzen AI MAX+ 395, AOCL 5.2.0, fork 2b87010c, upstream faer-v0.24.4), with a
Not fixed (needs the toolkit repo): with a thread environment that disagrees with |
shinaoka
changed the title
GPT-6.1-sol 敵対的レビュー（
|
The review's reproduction: one host and one revision, but the AOCL arm recorded under timing policy v1 (one best-of call, no priming) on CPUs 0-7, and the page accepted it and printed the MKL arm's "v2, 5 calls, median, priming 250 ms" above a table of two different measurements. `require_one_host` is now `require_one_protocol`. It compares, for every arm against the MKL arm: the host block; `timing_policy` (version, repetitions, statistic, minimum_untimed_priming_ms); the measured CPU sets per thread budget and the observer's affinity; `host.power` (boost on and off are incomparable, docs/protocol-v2-baseline.md); the revisions of library, upstream, harness and toolkit; and the cell grid -- for every cell shape an arm was asked for, its dtype and thread count must be one of the reference arm's, `skipped` rows included. The join key already contains those columns, so a disagreement never joins, and the dash that would leave is read as "that arm did not carry the cell". Every differing field is printed with both values, and nothing is written. Timestamps are not compared: the arms of a publication are recorded hours apart, so a session is defined by what the numbers depend on, and the page now says "one measurement protocol" instead of "in this session". Three counting and scope fixes in the same page: - the ratio beside a vendor row is taken against the fork of the arm the reference came from, so its two numbers come from one arm. The AOCL arm's own `faer-fork` row used to be discarded, and an OpenBLAS arm's fork could stand in for it, moving vendor/fork from 0.5 to 0.05 without either reference row changing; - a cell is counted as "falls back to MKL" only when the MKL row was measured; a cell with no vendor row at all is counted as that instead of claiming a number that does not exist, and "AOCL in N of M cells" counts resolved references, so "in 1 of 0 cells" is unreachable; - the reduction over process sets is a parameter. The vendor-reference page (`--aocl`) keeps the between-set median it needs to choose between two arms; a page without `--aocl` keeps the last process set it has always shown, so an existing page is not rewritten by a change to the join (a 3-set arm with fork 10/20/40 ms read `40 ms`/`2.500` at 8adb723 and `20 ms`/`5.000` on this branch). `tests/fixtures/comparison-8adb723-table.html` pins that table body byte for byte, and the real 1356-cell published page reproduces it exactly. The page also renders the statistic, repetition count and priming from the manifest, and states which reduction it applied, instead of hardcoding v2 into the prose. Tests: 10 new -- protocol refusal with every field named, timestamp alone accepted, dtype mismatch, boosted host, per-budget CPU sets refused and a budget only one arm measured accepted, the AOCL arm's own fork shown, an OpenBLAS arm not standing in for it, no-vendor not counted as a fallback, and the 8adb723 table fixture. `python3 -m unittest discover -s tests` -> 34 passed. review: gpt-6.1-sol (independent, pre-push) -- 10 findings, all checked against the source: 10 fixed (OpenBLAS fork precedence, harness/toolkit revisions, host.power, CPU sets per thread budget, skipped rows in the grid, resolved-reference count, manifest-name ordering, v2 hardcoded in prose, reduction not disclosed, "every field" vs early exits); delete list applied except `test_the_two_arm_page_is_unchanged`, which covers the empty-OpenBLAS-arm case the fixture does not. Noted, not fixed: `threads.cpu_sets` is compared as the recorded string, so two spellings of one CPU set would be refused rather than recognised.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Automated ( |
|
Converted to draft: GPT-6.1-sol posted four
I have not reproduced these yet; they are the reviewer's fixtures, and reporting them as fixed without a reproduction is what this PR already got burned by once. Until each has a test that fails on |
Adds the AOCL arm to
main(it lived only on the closed #52's branch), with the two rulesthat make it usable as a reference rather than a curiosity:
lapack-aoclonly on anAuthenticAMDhost, and says so in the data whenit does not;
names the library each cell used.
1. The host gate (harness)
AOCL is an x86-64 package built and dispatched for AMD's CPUs: AOCL-BLAS reports its kernel set
(
blis=zen5on this host) and libflame picks its AVX2/AVX-512 path from the same CPU-feature query.A number produced on any other CPU would be a library built for a different machine, published as
the vendor reference — so the arm now states the requirement instead of measuring anyway.
fbench/src/vendor.rshost_cpu_vendor()—vendor_idfrom/proc/cpuinfoon Linux,sysctl -n machdep.cpu.vendoronmacOS,
Errelsewhere. No default: a host that cannot be read is not AMD, because a defaultturns "we could not tell" into a measurement on the wrong machine.
amd_host_refusal(Result<String, String>)— the rule itself, split from the probe so it can bestated for a host the test is not running on (and so a reviewer reads the rule without reading
platform code).
host_refusal()— the arm's answer:Some(reason)forlink-aoclon a non-AMD host,Noneforevery other arm (
link-mkl,link-openblas,nonerun wherever their library loads).fbench/src/main.rsasks before touching any thread budget, and writes the row asskippedwiththe reason in the note, for every cell the run asked for:
Not a hard failure; not a silently missing row. It is the same contract the harness already uses for
a family the vendor library has no route for. The CSV schema is unchanged (no new column).
2. The per-cell vendor reference (page)
scripts/gen_comparison.py --aocl <run dir>:vendor.name=>lapack-aocl), and a cell's reference is the AOCL value when it is strictly smaller than theMKL one, MKL otherwise. A tie stays with MKL. A re-measured cell moves the reference with it.
referencecolumn names the library each cell's ratio uses, and both measured values stay inthe table (
MKL (ms),AOCL (ms)), so a reader sees the losing number — including the near-tiesthe selection decided.
marked
MKL*, with the arm's ownskippednotes listed under the table (eigh,svd_values,svd_thin,full_piv_luhave no LAPACK driver in this arm).MKL*= "an MKL-only statement, nota comparison of two libraries." A cell neither arm has a vendor row for is not a fallback: it
has no reference, the column shows
-, and the page and the log count it separately as "no vendorrow at all".
the timing policy, the measured CPU sets (per thread budget),
host.power, the revisions oflibrary/upstream/harness/toolkit, and the cell grid (dtype and thread count per cell shape,
skippedrows included) are compared across the arms, and a difference is refused with everydiffering field printed on both sides. Timestamps are not compared — the arms of a publication are
recorded hours apart — so the page says "under one measurement protocol" rather than "in this
session".
its two numbers are two rows of one measurement; an arm's
faer-forkrow is never discarded andanother arm's is only used when that arm had none.
--openblasbecomes optional (its columns appear when given). Without--aoclno referencecolumn and no arm are added, and the columns keep the values this script wrote before: the
between-set median is the vendor-reference page's reduction, a page without
--aoclkeeps thelast process set (
tests/fixtures/comparison-8adb723-table.htmlpins that table body byte forbyte, and the published 1356-cell page reproduces it). Apart from that the page differs in the
summary fix below and in wording/CSS.
Drive-by fix, same file: the summary line counted and took the median of the wrong index —
r[6]is the upstream/fork ratio, not the OpenBLAS one — so the current published line says1356 cells ... OpenBLAS 1.956where 972 cells carry an OpenBLAS row and their median is1.230.That number is on the live page today; nothing else about the OpenBLAS columns changes.
3. Evidence
Harness, on an AMD host (EVO-X2, AOCL 5.2.0, MKL 2026.2, fork
2b87010c).scripts/build_for_faer_rev.sh --vendor aoclbuildslink-aoclclean; the 5 warnings are all incode this PR does not touch (unused LAPACKE declarations, the unused
vendor::nancheckwrapper).cargo test --features link-aocl,fork-par-shape,fork-team→ 11 passed, includingvendor::tests::the_amd_rule_names_the_vendor_it_found,vendor::tests::the_vendor_comes_from_the_vendor_id_field_only,vendor::tests::only_the_aocl_arm_has_a_host_requirement,tests::a_refused_row_is_written_as_skipped_with_its_reason,tests::the_host_rule_is_asked_of_the_vendor_row_only.the arm still measures on that host:
run --rows lapack-aocl --n 64→okrows with times.the non-AMD path end to end: the real binary, with
LD_PRELOADredirecting/proc/cpuinfotoa file that says
vendor_id: GenuineIntel(user namespaces are disabled on that host, so a shimis how the host is faked; it touches nothing in the product):
Page. The 2026-11-07 AOCL/MKL session on EVO-X2 (n=1024, f64, reps 5, 250 ms priming, three
process sets per cell, the fork measured in both processes) assembled into two run directories — the
driver's CSVs are raw and
record_run.pydoes not know the AOCL vendor yet, so the two manifestsare transcribed from the run's own
fbench infooutput and the machine:The log line this branch printed before the review round was
and it did not add up: 12 + 28 is neither the 52 cells nor the 30 counted. Both counting bugs are
fixed (a cell with no MKL row was counted as a fallback to it, and the AOCL arm's own
faer-forkrow was dropped when the MKL arm carried none), so the fixed script renumbers these cells — and the
numbers are deliberately not restated here: the 2026-11-07 recording is not on this machine
(
find data -name run.yaml | xargs grep -l aocl→ nothing), and a count written from memory isexactly what this round is fixing. The three numbers the fixed script prints are exclusive (AOCL
reference / fallback to a measured MKL row / no vendor row) and the fixtures in §5 reproduce them.
AOCL is the faster vendor row in 12 of the 24 cells both arms carry. That count and the table below
come from the recorded session and are unaffected by this round: the choice between the two vendor
rows is the same rule, unchanged. Ratios below are AOCL/MKL
(median of the three process sets; below 1.00 means AOCL is faster, bold = AOCL is the reference):
Two notes on that table, because they are the reason this is computed and not declared:
the AOCL arm wins f-order
cholesky(3 cells); against MKL it also wins everygemmand f-orderqrat these thread counts. A best-of written as a cell list from the first observation wouldhave been wrong by 9 of 12 cells — which is exactly the failure mode the data-driven choice
avoids.
gemm f 8T0.982,qr_factor f 8T0.998) are within a few percent and flipbetween process sets; the page shows both values beside the choice, so it is visible that the
reference for those cells is a coin-flip rather than a finding.
4. Docs
docs/vendor-reference.md— what the reference is, why it is chosen per cell from the data, whyonly one host, why AOCL is
AuthenticAMD-only (x86-64 package, AMD kernel dispatch), whatMKL*means, how to regenerate a page, and what the page still does not claim.
README.md— the AMD-only requirement as a reproducibility bullet of the AOCL arm, and a sectionpointing at the new page.
5. Tests
tests/test_comparison_aocl.py, 16 tests over fixtures in the shaperecord_run.pywrites(
python3 -m unittest discover -s tests→ 34 passed withcampaign-toolkitinstalled):MKL*, and the page carries the arm'sown reason string;
with the reason;
8adb723(
tests/fixtures/comparison-8adb723-table.html) — the test that replaces "noreferencecolumn"with a check that would fail if every ratio became a dash;
CPUs 0-7 in another year is refused, with each differing field named on both sides;
measured is accepted, and the same budget on other CPUs is refused;
host.power);The harness side of the non-AMD case is covered by the
cargo testcases above.Review round
review: gpt-6.1-sol 10 findings, all checked against the source and fixed; no unfixed findingAn independent post-implementation review (read-only, on the finished diff) raised 10 findings and a
delete list, all applied in
31642dbexcept the one noted below:faer-forkcould outrank the AOCL arm's for the vendor ratio (0.5 → 0.05 withno reference row changing) — the ratio now comes from the reference arm;
protocol identity, together with
host.power(boost on/off are incomparable) and the revisionsthemselves;
shared thread budget;
skippedrows, so an arm asked for an f64 cell and skipping it passed as an armasked for f32 — skipped cells are in the grid now;
count now uses resolved references;
last_setfollowed the manifest's file order where8adb723sorted the names — sorted, so thefrozen page is exact for more than 9 process sets;
rendered from the manifest, and the page states which reduction it applied;
now arrive in one report.
Not fixed, stated:
threads.cpu_setsis compared as the recorded string, so two spellings of one CPUset would be refused rather than recognised as equal.
Not changed
No measurement numbers, thresholds, reps/priming, CPU sets or pins; the CSV schema; the MKL and
OpenBLAS arm output.
AGENTS.md/CLAUDE.mduntouched. Merging is left to the reviewers.