Pin a baseline CPU target for Phoenix NVHPC builds - #1864
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
cmake/GPU.cmake may still add -march=native, so -tp=px does not guarantee a portable binary.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR pins Phoenix NVHPC builds to a portable baseline CPU target to reduce cross-node SIGILL failures.
Changes:
- Adds
-tp=pxfor Fortran, C, and C++ Phoenix GPU builds. - Applies the flags through
toolchain/modules.
File summaries
| File | Summary |
|---|---|
toolchain/modules |
Configures portable Phoenix NVHPC CPU targeting; native tuning may still override it. |
Review details
Suppressed comments (1)
toolchain/modules:52
- Because
p-gpuis also loaded by the Phoenix GPU benchmark path (.github/workflows/common/bench-pair.sh), this retargets benchmark binaries as well as the case-optimization/test binaries. The resulting host-side timings are not directly comparable with the existing Phoenix baseline figures indocs/documentation/expectedPerformance.md; please either scope this flag to the failing build path or explicitly version/update the Phoenix benchmark baseline before merging.
p-gpu FFLAGS=-tp=px CFLAGS=-tp=px CXXFLAGS=-tp=px
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # (the case-optimization lane submits the prebuild and the run as separate allocations), so a | ||
| # natively targeted binary can meet a node that lacks an instruction it uses and die with SIGILL. | ||
| # preflight.sh probes with syscheck, which is too small to trip it. Pin a baseline instead. | ||
| p-gpu FFLAGS=-tp=px CFLAGS=-tp=px CXXFLAGS=-tp=px |
There was a problem hiding this comment.
Good catch to check - cmake/GPU.cmake:186-207 does add -march=native to every Fortran target in Release, and nvfortran accepts the flag (rc 0), so CHECK_FORTRAN_COMPILER_FLAG succeeds and it lands on the command line after CMAKE_Fortran_FLAGS. But measured on nvfortran 25.5, -tp wins over a later -march=native rather than the other way round.
Same subroutine, -O3 -fast, object disassembled:
| flags | AVX-style mnemonics | ymm/zmm | md5 |
|---|---|---|---|
-tp=px |
26 | 22 | ee8f3042 |
-tp=zen3 |
16 | 19 | 71b4755c |
-march=native |
16 | 19 | 61e15704 |
-tp=px -march=native |
26 | 22 | 5f21ba55 |
-march=native on its own reproduces -tp=zen3 exactly on this Zen 3 host, so it is honoured rather than ignored - but combined with -tp=px the emitted ISA profile is identical to -tp=px alone. So the baseline is not defeated here.
Worth adding what the same measurement says about whether this PR would help at all: neither target emits AVX-512 on this host (zmm count 0 for both), and -tp=px still emits ymm, so "px" is not an SSE2 baseline. It caps the ISA rather than eliminating vectorisation, which is the relevant part - a build host with AVX-512 would emit zmm under native tuning and those are the instructions an older node would fault on.
That said, #1865 excludes the offending node directly, at no performance cost and following the existing node_exclude precedent, so it is the better immediate fix. This PR is the systemic version and carries a real cost (Phoenix Bench numbers will move), so it should probably wait on whether the exclusion holds.
An experiment, not a settled fix - see the tradeoff below before merging.
What this is chasing
#1845's Case Opt lane has failed four times over 24 hours, always the same way:
All four attempts ran on node
atl1-1-01-002-28-0; no passing Case Opt job on any PR has run on that node. The change it carries cannot be the cause:num_probes_maxappears nowhere insrc/common/orsrc/pre_process/, the generated Fortran is byte-identical at 10 and 64, and I reproduced CI's exact build (same sluggpu-mp-0e981924c0, case-optimized,--gpu mp, MPI) under both NVHPC 24.1 and 25.5 locally, wherepre_processruns clean. What #1845 does do is changem_constants.fpp, which forces a full rebuild of everything downstream.The mechanism the repo already documents:
run_case_optimization.sh: "these scripts nuke and rebuild build/ themselves (Phoenix does so precisely because its compute nodes are heterogeneous)".node-exclude.sh: the preflight is meant to catch "SIGILL from a binary built for another microarchitecture".toolchain/modulessets no-tpforp-gpu, so nvfortran targets the build host's native CPU.prebuild-case-optimization.shandrun_case_optimization.shas separatesubmit-slurm-job.shcalls, so the build and the run can land on different nodes. Raise the probe limit from 10 to 64 #1845's logs show exactly that - built onatl1-1-02-*, run onatl1-1-01-002-28-0.preflight.shprobes the node withsyscheck, which is small enough that it does not use the instructions that differ. It passes,MFC_FAULT_NODEis never emitted, and the node is never excluded.So a natively targeted binary built on a newer node meets an older one and dies in
pre_process, and the guard that exists for precisely this cannot see it.The tradeoff, which is the part to decide
-tp=pxis the fully generic x86-64 target: no AVX, AVX2 or AVX-512. For the GPU lanes the host code is not where the time goes, butpre_processandpost_processare CPU-only and will get slower, and the Phoenix Bench numbers will move. If the benchmark series is meant to be comparable over time, that matters more than the build time does.Two alternatives that keep vectorization, either of which I would prefer if we can get the information:
-tp=skylake/-tp=zen2is correct and costs almost nothing on the newer nodes. I do not know Phoenix's node inventory; you likely do.-tp=<a>,<b>, emitting multiple code paths with runtime dispatch - full speed on new nodes, correct on old ones, at the cost of binary size and compile time.There are also two fixes that leave the flags alone: make
preflight.shprobe a real binary (pre_process) rather thansyscheck, so the existing node-exclusion machinery actually fires; or run the case-opt prebuild and run in one allocation so they cannot land on different nodes. Those are better targeted but larger changes.How to tell whether it worked
Merging this and re-running #1845's Case Opt lane is the test. Until the lane lands on
atl1-1-01-002-28-0again, a pass proves nothing - the failure needs that node. Worth re-running a few times, or excluding everything else, to force it there.