Add CI: build matrix for Linux/macOS/Windows on x64 and ARM64 - #6
Merged
Conversation
tests/downstream_find_package/ is a standalone CMake project that resolves HPTT the way a real dependent does -- through find_package(hptt CONFIG) and the hptt:: imported targets -- rather than through the in-tree target names. It therefore exercises the generated hpttConfig.cmake and hpttTargets.cmake, which nothing in the repository covered before. The consumer transposes randomly filled tensors and compares against a naive index-arithmetic reference, so linking is not the only thing it proves: all four instantiated scalar types, dimensionalities 2 through 5, non-trivial permutations, non-zero beta, complex alpha/beta, and single- and multi-threaded execution all have to produce the right answer. It builds one executable per available target, so it configures against a static-only HPTT as well. On Windows the DLL is copied next to consumer_dyn, which is how the loader finds it there; TARGET_RUNTIME_DLLS is empty on rpath platforms, so no WIN32 guard is needed. Co-Authored-By: Claude <noreply@anthropic.com>
The repository had no CI. Nothing checked that a change kept HPTT
building on a second platform, and nothing checked that the CMake package
it exports is actually consumable.
Add .github/workflows/ci-build.yml, a six-way matrix over
{Linux, macOS, Windows} x {x86-64, ARM64}. Each job builds HPTT with
Ninja, then configures tests/downstream_find_package twice and runs it
both times: once against the build tree via hptt_DIR, which exercises
export(TARGETS ...) plus the build-directory hpttConfig.cmake, and once
against an install prefix via CMAKE_PREFIX_PATH, which exercises
install(EXPORT ...). Each job also builds HPTT_BUILD_SHARED=OFF, and the
Windows jobs assert that hptt.lib, hptt_dyn.lib and hptt.dll all exist,
which is the artifact-naming rule the Ninja generator enforces.
Triggers are `pull_request` with no branches filter, so a pull request
against any target branch is built, and `push` on main for merges.
Concurrency is grouped per pull request with cancel-in-progress, so
pushing a new commit cancels the queued and running jobs of the previous
one; pushes to main key on run_id instead so merged commits each keep
their result.
ccache is wired in through CMAKE_<LANG>_COMPILER_LAUNCHER with a
per-matrix-entry cache key. The Windows jobs additionally set CMP0141 and
MSVC_DEBUG_INFORMATION_FORMAT=Embedded, because ccache cannot cache
compilations that write debug info to a shared .pdb.
macOS installs libomp, since AppleClang ships no OpenMP runtime and
find_package(OpenMP REQUIRED) fails without it.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51dcba0754
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The GNU branch appended `-march=x86-64 -mtune=generic` to every non-Apple,
non-IBM build without checking the target architecture. `x86-64` is the
baseline x86 -march value and is not accepted anywhere else, so a GCC
build on aarch64 fails on the first translation unit:
cc1plus: error: unknown value 'x86-64' for '-march'
cc1plus: note: valid arguments are: armv8-a armv8.1-a ... native
Guard the flag on CMAKE_SYSTEM_PROCESSOR naming an x86 target. Other
architectures fall through to the compiler's own default, which is the
correct portable baseline. FINE_TUNE=ON is unaffected: `-march=native` is
valid on aarch64.
Co-Authored-By: Claude <noreply@anthropic.com>
The consumer resolves HPTT with find_package(hptt CONFIG), and
hpttConfig.cmake re-runs find_dependency(OpenMP REQUIRED) in the
consumer's own scope -- OpenMP::OpenMP_CXX is a PUBLIC link dependency of
the hptt targets, so a consumer has to find it too. AppleClang ships no
OpenMP runtime, so without a pointer at the brew libomp that call fails:
Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES)
...
build/hpttConfig.cmake:34 (find_dependency)
Setting OpenMP_ROOT only when building HPTT was not enough. Pass it to
both consumer configure steps as well, which is also what a real macOS
consumer of an installed HPTT has to do.
Co-Authored-By: Claude <noreply@anthropic.com>
The ENABLE_ARM branch appended -mfpu=neon on every non-MSVC compiler. It is a 32-bit ARM flag: on a 64-bit ARM target Advanced SIMD is part of the baseline ISA, so there is nothing to switch on and no -mfpu option to switch it with, and GCC on aarch64 rejects the flag outright. ENABLE_ARM was therefore unusable on aarch64 -- the one architecture whose Neon kernels it exists to select. Guard the flag on CMAKE_SYSTEM_PROCESSOR not naming a 64-bit ARM target. HPTT_ARCH_ARM is still defined in every case, which is what actually selects the Neon micro-kernels in src/transpose.cpp and sets REGISTER_BITS=128. Co-Authored-By: Claude <noreply@anthropic.com>
The ARM matrix entries built HPTT's default configuration, which on those targets defines neither HPTT_ARCH_AVX nor HPTT_ARCH_ARM and so compiles the generic scalar micro-kernel. The Neon path in src/transpose.cpp -- the reason ARM support exists -- went uncompiled and unrun on every platform. Add an arch_flags matrix field and pass -DENABLE_ARM=ON on linux-arm64, macos-arm64 and windows-arm64. The full pipeline then runs against the Neon build: install, both downstream consumers, and the correctness comparison against the naive reference. Because HPTT_ARCH_ARM is a PUBLIC compile definition, this also checks that consumers of the exported package see the same REGISTER_BITS the library was built with. The HPTT_BUILD_SHARED=OFF step deliberately keeps the default flags, so the generic scalar configuration stays covered on ARM too. Co-Authored-By: Claude <noreply@anthropic.com>
The previous guard read `NOT MSVC AND NOT <64-bit ARM>`, which invited the reading that MSVC on ARM64 would otherwise accept `-mfpu=neon`. It would not, and neither clause is about that: the architecture clause already excludes every 64-bit ARM target, MSVC included. State the actual rule instead. `-mfpu=neon` is a GCC/Clang spelling that applies only to 32-bit ARM, so test for a GNU/Clang compiler and a target that is not 64-bit ARM. Behaviour is unchanged on every platform CI builds: MSVC ARM64, GCC aarch64 and AppleClang arm64 all skipped the flag before and skip it now, and 32-bit ARM GCC/Clang still receives it. Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Based on #5 — targets
msvc-build, notmain. Independent of #7; the two only share a base.The repository has no CI today. Nothing checks that a change keeps HPTT building on a second platform, and nothing checks that the CMake package it exports is actually consumable.
Status: all six jobs green
Run 30481654763:
ubuntu-latestubuntu-24.04-armmacos-15-intelmacos-latestwindows-latest(MSVC + Ninja)windows-11-arm(MSVC + Ninja)What each job does
Builds HPTT, then configures
tests/downstream_find_packagetwice and runs it both times:-Dhptt_DIR=<build dir>, exercisingexport(TARGETS ...)plus the build-directoryhpttConfig.cmake-DCMAKE_PREFIX_PATH=<prefix>, exercisinginstall(EXPORT ...)Each job additionally builds
HPTT_BUILD_SHARED=OFF, and the Windows jobs asserthptt.lib,hptt_dyn.libandhptt.dllall exist — the artifact-naming rule the Ninja generator enforces.The consumer is a correctness test, not just a link test:
consumer.cpptransposes randomly filled tensors and compares against a naive index-arithmetic reference across all four instantiated scalar types, 2D through 5D, non-trivial permutations,beta != 0, complexalpha/beta, and 1 and 4 threads. A green job means the exported package links and the library computes correct results on that platform.It builds one executable per available target, so it also configures against a static-only HPTT. On Windows the DLL is copied next to
consumer_dyn;TARGET_RUNTIME_DLLSis empty on rpath platforms, so noWIN32guard is needed.Two real bugs the first run found
The first run (three jobs red) surfaced two genuine defects, not workflow mistakes. Both are fixed here, each in its own commit.
1.
-march=x86-64was applied on every architecture. The GNU branch appended-march=x86-64 -mtune=genericto all non-Apple, non-IBM builds without checking the target, so GCC on aarch64 failed on the first translation unit:HPTT does not currently build on Linux ARM64 at all. The flag is now guarded on
CMAKE_SYSTEM_PROCESSORnaming an x86 target; other architectures take the compiler's default.FINE_TUNE=ONis unaffected —-march=nativeis valid on aarch64.2. macOS consumers could not resolve OpenMP.
hpttConfig.cmakere-runsfind_dependency(OpenMP REQUIRED)in the consumer's scope, becauseOpenMP::OpenMP_CXXis a PUBLIC link dependency of the hptt targets. SettingOpenMP_ROOTonly when building HPTT was not enough; both consumer configure steps now pass it too. This is not a workaround — it is what a real macOS consumer of an installed HPTT has to do, and it is worth knowing that the exported package places that requirement on its users.Triggers and cancellation
pull_requestdeliberately has nobranches:filter, so a PR against any target branch is built — which is what makes a stacked PR like this one get checked at all.Concurrency groups per PR with
cancel-in-progress, so pushing a new commit cancels the previous run's queued and in-flight jobs. Pushes tomainkey onrun_idinstead and never cancel each other, so every merged commit keeps its own result.ccache
Wired through
CMAKE_<LANG>_COMPILER_LAUNCHERrather than by shadowing the compiler onPATH, so it keeps working with the absolute compiler paths CMake records. Cache key is per matrix entry; the six platforms never share objects.The Windows jobs also set
CMP0141+CMAKE_MSVC_DEBUG_INFORMATION_FORMAT=Embedded. ccache cannot cache compilations that write debug info into a shared.pdb, so without this every Windows compile would be a cache miss.Note on
ENABLE_ARMThe ARM jobs deliberately do not pass
-DENABLE_ARM=ON. On aarch64 that path appends-mfpu=neon, an ARM32-only flag GCC rejects there. So the ARM jobs build HPTT's generic scalar kernels withREGISTER_BITS=256, which is the current default on those platforms. MakingENABLE_ARMactually usable on aarch64 is a separate change; this PR establishes the coverage that would catch it.