Repository navigation
Conversation
Adds first AMD GPU support for Marian via a Strategy-A CUDA-to-HIP port behind
option(USE_HIP); the NVIDIA build is unchanged (every divergence is
USE_HIP-guarded). Authored with the assistance of Claude (an AI assistant).
Review order: start with the single compat header
src/tensors/gpu/cuda_to_hip.h (runtime cuda*->hip* aliases plus a
CUDA_VERSION>=11000 sentinel) and the forwarding shims under
src/tensors/gpu/hip_compat/ (cublas_v2/cublasLt/cusparse/curand/fp16/...). Then
the CMake: top-level CMakeLists.txt adds the USE_HIP branch (enable_language
HIP, find the ROCm math libs, a cupoch-style cuda_add_library shim macro that
marks the .cu and mixed .cpp TUs LANGUAGE HIP, force-include the compat header).
Then the library swaps in prod.cpp / prod_sparse_cu11.h and the FP16 steering in
common/types.h + functional/{operators,defs,tensor}.h. Finally the wave64
reduction fix in tensors/gpu/topk.cu and translator/nth_element.cu.
Root cause of the key correctness fix: topk.cu and nth_element.cu reduced shared
memory with a __syncthreads() tree down to 32 lanes, then an UNROLLED
warp-synchronous tail (UNROLL_MAXARG_LOOP 32..1) with no barrier on non-volatile
shared memory. That tail assumes the bottom 32 lanes run in lockstep, which
holds on a 32-wide NVIDIA warp but NOT on the low 32 lanes of a CDNA wave64
wavefront -- so the argmax becomes racy and non-deterministic. nth_element is
the beam-search top-k on the decode path, so this would silently corrupt
translations rather than fail loudly. Fix (USE_HIP-guarded): drop the unrolled
tail and run the __syncthreads()-synchronized tree to s>0. Same compare order, a
block-wide barrier at every step, correct on any wave size; the CUDA path is
byte-identical.
hipBLASLt deltas (USE_HIP-guarded in prod.cpp): the
MATMUL_PREF_MIN_ALIGNMENT_*_BYTES preference attrs do not exist and are dropped;
the matmul desc uses HIPBLAS_COMPUTE_32F and a HIP_R_32F scale type for both
fp16 and fp32 (HIPBLAS_COMPUTE_16F / a 16F scale type are rejected);
cublasSetMathMode(TENSOR_OP) is unsupported on gfx90a so the tensor-op-math
helpers are no-ops; hipBLASLt needs its own handle (CUDA shares one with
cuBLAS). FP16 steering keys on __HIPCC__/__HIP_DEVICE_COMPILE__ rather than a
global __CUDACC__ define (which would misroute rocThrust to its CUDA backend).
Scope: built and validated with USE_CUDNN=OFF and USE_NCCL=OFF -- the cuDNN
convolution/pooling path (all #ifdef CUDNN-gated) is kept CUDA-only (a MIOpen
port is a separable follow-up), and NCCL multi-GPU collectives (RCCL swap) are a
follow-up; neither is needed by the default dense Transformer. The cuSPARSE
sparse-attention CSRProd path is also a documented follow-up (hipSPARSE SpMM
order/alg work).
Test Plan:
Built for gfx90a (ROCm 7.2.1):
```
git submodule update --init src/3rd_party/sentencepiece
cmake -S . -B build-hip -G Ninja -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx90a \
-DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCOMPILE_CUDA=ON \
-DUSE_CUDNN=OFF -DUSE_NCCL=OFF -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON \
-DCMAKE_BUILD_TYPE=Release -DCOMPILE_TESTS=ON -DUSE_MKL=OFF \
-DUSE_TCMALLOC=OFF -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DBUILD_ARCH=native
cmake --build build-hip -j
```
Ran the GPU unit suites on a real gfx90a:
```
./src/tests/units/run_graph_tests # all pass
./src/tests/units/run_attention_tests # all pass
./src/tests/units/run_transformer_tests # all pass
./src/tests/units/run_operator_tests # 284/287 (only cuSPARSE csr-dot fails)
```
operator_tests covers the topk/argmax/argmin/sort kernels (exact values).
End-to-end beam-search decode (exercises nth_element.cu) on gfx90a: trained a
tiny Transformer to convergence, then
```
marian-decoder -m model.npz -v v.src.yml v.tgt.yml -i test.src -b 6 --devices 0
```
run twice is bit-identical (deterministic -> the wave64 race is gone) and equals
the --cpu-threads 1 decode (correct). gMaxElement/gMaxElementUpdate confirmed
dispatched on gfx90a via AMD_LOG_LEVEL=3.
Enable building on Windows with clang++ targeting x86_64-pc-windows-msvc (non-MSVC driver, MSVC ABI). Seven concrete changes: - CMakeLists.txt: replace hardcoded -fPIC with a WIN32-conditional FPIC_FLAG; on Windows it is empty (clang rejects -fPIC for the MSVC target triple). - CMakeLists.txt: add WIN32_UNICODE_FLAGS on Windows (UNICODE/_UNICODE, DWIN32/_WINDOWS, -D_DLL -D_MT --dependent-lib=msvcrt) so that LoadLibrary() maps to LoadLibraryW and the CRT is picked up. - CMakeLists.txt: remove the Clang exclusion from -march=native; clang++ supports -march= fine on all targets. - CMakeLists.txt: add Shlwapi.lib in the else(MSVC) path under WIN32 guard; pathie-cpp calls PathMatchSpecW which lives there. - CMakeLists.txt: add -Wno-string-plus-int / -Wno-unused-private-field to ALL_WARNINGS for WIN32+Clang (spdlog Windows API, threadpool.h lambda). - CMakeLists.txt: add -Wno-string-plus-int to MARIAN_HIP_NO_WARN (same spdlog warning fires in HIP TUs). - src/3rd_party/CMakeLists.txt: guard the Clang -fPIC flags on libyaml-cpp and pathie-cpp with NOT WIN32. - src/3rd_party/sentencepiece/src/CMakeLists.txt: add NOT WIN32 guard on the -fPIC flag in the if(NOT MSVC) block. - src/3rd_party/sentencepiece/src/trainer_interface.cc: change constexpr to const for kAnyType (Clang strict constexpr rejects out-of-range enum cast). These changes are guarded by WIN32 and/or CMAKE_CXX_COMPILER_ID matches "Clang" conditions; the Linux and MSVC paths are byte-identical. This work was authored with Claude (Anthropic). Test Plan: cmake -S . -B build-gfx1201-win -G Ninja \ -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1201 \ -DCMAKE_C_COMPILER=<rocm_devel>/lib/llvm/bin/clang.exe \ -DCMAKE_CXX_COMPILER=<rocm_devel>/lib/llvm/bin/clang++.exe \ -DCMAKE_HIP_COMPILER=<rocm_devel>/lib/llvm/bin/clang++.exe \ -DCOMPILE_CUDA=ON -DUSE_CUDNN=OFF -DUSE_NCCL=OFF \ -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON -DCMAKE_BUILD_TYPE=Release \ -DCOMPILE_TESTS=ON -DUSE_MKL=OFF -DUSE_TCMALLOC=OFF -DUSE_DOXYGEN=OFF \ -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DBUILD_ARCH=native \ -DOpenBLAS_LIBRARIES=<rocm_devel>/lib/host-math/lib/rocm-openblas.lib \ -DOpenBLAS_INCLUDE_DIRS=<rocm_devel>/lib/host-math/include/openblas cmake --build build-gfx1201-win -j32 # Runs successfully, 20 link targets produced. # GPU tests: graph_tests 10/10 PASS on gfx1201 (RDNA4, RX 9070 XT).
On Windows with TheRock ROCm 7.14, libhipblaslt.dll faults inside Tensile's FP8 type check (hipblaslt_f8::is_inf) whenever marian's Affine() drives the fused GEMM+bias through the hipblasLtMatmul descriptor API (hipblasLtMatmulAlgoGetHeuristic). The crash is independent of the input data type (it triggers for fp32 and fp16 alike) and wedges every Affine-bearing path: the operator "affine transformation" test, attention, transformer, and end-to-end train/decode. The fix routes Affine on Windows + ROCm around the hipBLASLt epilogue: compute the matmul with the plain GEMM (ProdTyped, which lowers to hipblasGemmEx / hipblasSgemm) and then add the broadcast bias with the existing BiasAdd kernel, applying the same optional ReLU epilogue. This is exactly the pre-CUDA-11 fallback already present in this file, so the numerical result is identical to the hipBLASLt epilogue. The change is guarded by _WIN32 && USE_HIP; Linux and CUDA keep the hipBLASLt path byte for byte (the post-call misalignment BiasAdd that belongs to the hipBLASLt path is likewise guarded off on Windows + ROCm since the new affineTyped already applies the bias). This was developed with the assistance of Claude, an AI assistant. Test Plan: Built for gfx1201 (RX 9070 XT, RDNA4) with the TheRock all-clang Windows toolchain and ran the marian GPU unit tests, pinned to one GPU: ``` cmake --build agent_space/marian-build-gfx1201 -j24 HIP_VISIBLE_DEVICES=0 ROCBLAS_USE_HIPBLASLT=0 \ run_operator_tests.exe "Expression graph supports basic math operations (gpu)" HIP_VISIBLE_DEVICES=0 ROCBLAS_USE_HIPBLASLT=0 run_attention_tests.exe "*Attention (gpu)" HIP_VISIBLE_DEVICES=0 ROCBLAS_USE_HIPBLASLT=0 \ run_transformer_tests.exe "Expression graph supports basic math operations (gpu)" ``` operator_tests (gpu): 200/202 assertions pass; the affine transformation section passes (previously SIGSEGV). attention (gpu): 2/2 pass. transformer (gpu): 1/1 pass. The 2 remaining operator failures are the pre-existing deferred cuSPARSE csr-dot path, unrelated to this change. A tiny Transformer trained and beam-search decoded on gfx1201 (Affine in the FFN forward and backward) converges and is run-to-run deterministic.
Drop internal project labels from the ROCm/HIP shim comments and the CMake status message while keeping the technical descriptions intact. Comment-only change; authored by Claude.
Replace "byte-identical" in three port comments with plainer, accurate wording. The CUDA path is left unchanged when USE_HIP=OFF; "byte-identical" overclaimed and was not the right description. No code logic changes. Authored with assistance from Claude. Test Plan: Comment-only change; no build or test impact.
The ROCm/HIP path force-set CMAKE_HIP_ARCHITECTURES to gfx90a before enable_language(HIP) whenever it was unset, which preempted CMake's own host-GPU detection: a build on a non-gfx90a AMD GPU that did not pass an explicit -DCMAKE_HIP_ARCHITECTURES silently produced gfx90a code objects that fail to load at runtime. Removing the pin lets enable_language(HIP) honor an explicit -DCMAKE_HIP_ARCHITECTURES, otherwise auto-detect the host GPU(s), and otherwise error on a host with no AMD GPU. Test Plan: Configured and built with an explicit -DCMAKE_HIP_ARCHITECTURES=gfx90a; the produced device code objects are byte-identical to the previous pinned build (the pin only set the same value the -D now supplies). Authored with the Claude AI assistant.
jeffdaily
added a commit
to AMD-Ecosystem/moat
that referenced
this pull request
Aug 10, 2026
jeffdaily
added a commit
to AMD-Ecosystem/moat
that referenced
this pull request
Aug 10, 2026
The upstream sweep ran today (73 records, 0 drifted) and marian-dev's approved port was submitted upstream (marian-nmt/marian-dev#1043), which moves its README row. Authored with Claude. Test Plan: ``` python3 utils/upstream.py --dry-run python3 utils/upstream.py --publish --apply python3 utils/gen_readme.py ```
jeffdaily
marked this pull request as draft
August 10, 2026 15:49
Author
|
Converting to draft while we port additional features (sparse matrix, NCCL, and cuDNN paths). Will mark ready for review when complete. |
The CSR sparse-dense product produced wrong results on AMD GPUs for float and aborted outright for half, which is what kept the csr-dot operator test failing there. Two causes, both in prod_sparse_cu11.h. First, the multiplication was issued only when SpMM_bufferSize reported a non-zero scratch requirement. cuSPARSE always wants scratch for CSR_ALG2, so on NVIDIA that condition is invariably true and the shortcut is invisible; hipSPARSE needs none for these shapes and reports zero, so the product was skipped altogether and C kept whatever it held. The multiplication now always runs and only the scratch allocation is conditional. Second, hipSPARSE has no uniform half-precision SpMM at all. Half operands are accepted only by its mixed-precision mode, which pairs half sparse and dense inputs with a float result matrix and float alpha/beta; asking for a half result returns HIPSPARSE_STATUS_NOT_SUPPORTED. The half instantiation therefore multiplies into a float scratch matrix that is cast back into C, seeding that scratch from C first when beta is non-zero so the accumulate form still holds. The scalars handed to SpMM are floats on this path, matching the compute type the descriptor already declared -- previously a half alpha was passed against a float compute type, so the library read four bytes out of a two-byte value. Review the compute-type and result-matrix selection at the top of TypedSparseGemm first; the body of CSRProd then follows from it. The CUDA path is unchanged apart from the buffer-size condition, which cannot alter its behaviour. Authored with the assistance of Claude, an AI assistant by Anthropic. Test Plan: ``` cmake -S . -B build-hip -G Ninja -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCOMPILE_CUDA=ON \ -DUSE_CUDNN=OFF -DUSE_NCCL=OFF -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON \ -DCMAKE_BUILD_TYPE=Release -DCOMPILE_TESTS=ON -DUSE_MKL=OFF \ -DUSE_TCMALLOC=OFF -DUSE_DOXYGEN=OFF -DBUILD_ARCH=native cmake --build build-hip -j ./build-hip/src/tests/units/run_operator_tests ``` On a Radeon Pro W7800 (gfx1100, ROCm 7.2.3) the csr-dot section passes for both float and half. The float GPU case is 202/202 assertions.
With the sparse product fixed the operator test case runs to completion for the first time on AMD GPUs, and the half-precision affine transformation section fails there: affine() returns plus and minus infinity where the equivalent dot()+bias returns the right values, and affineWithReluDropout() returns the bias alone. The matmul descriptor declares a 32F scale type on this platform, because hipBLASLt rejects a 16F one, but the half overload kept passing pointers to half alpha and beta. The scale type is what tells the library how wide a value to read through those pointers, so it read four bytes out of a two-byte half and scaled the product by whatever adjacent stack bytes happened to be -- large enough to overflow to infinity in one call, near enough to zero to erase the product in the other. The overload now widens both scalars to float, so they match the scale type the descriptor asked for. The path taken on NVIDIA, where the scale type follows the matrix type, is untouched. Authored with the assistance of Claude, an AI assistant by Anthropic. Test Plan: ``` cmake --build build-hip -j ./build-hip/src/tests/units/run_operator_tests ``` On a Radeon Pro W7800 (gfx1100, ROCm 7.2.3): 603 assertions in 4 test cases, all passing, including the half-precision GPU case that previously failed the three affine assertions.
Multi-GPU training needed USE_NCCL=OFF on AMD GPUs, which left the NCCLCommunicator out of the build entirely and limited training to one device. RCCL is ROCm's collectives library and keeps NCCL's API, names and semantics, so training/communicator_nccl.h compiles and runs against it unchanged; the only difference is the header spelling, which the forwarding header alongside the other compatibility headers handles. The build now finds RCCL through its CMake package and links it, and the vendored NCCL submodule -- an NVIDIA-only make build that hipcc cannot drive -- is skipped, so nothing needs to be checked out for it. The two stream-lifetime entry points the communicator uses are added to the runtime alias list. USE_NCCL=ON, the default, is now the working configuration on both platforms. Authored with the assistance of Claude, an AI assistant by Anthropic. Test Plan: ``` cmake -S . -B build-hip -G Ninja -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCOMPILE_CUDA=ON \ -DUSE_CUDNN=OFF -DUSE_NCCL=ON -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON \ -DCMAKE_BUILD_TYPE=Release -DCOMPILE_TESTS=ON -DUSE_MKL=OFF \ -DUSE_TCMALLOC=OFF -DUSE_DOXYGEN=OFF -DBUILD_ARCH=native cmake --build build-hip -j ./build-hip/marian --type transformer -t train.src train.tgt -m model.npz \ --vocabs vocab.src.yml vocab.tgt.yml --dim-emb 64 --transformer-dim-ffn 128 \ --transformer-heads 2 --enc-depth 2 --dec-depth 2 --after 3000u \ --devices 0 1 2 3 --mini-batch 32 --sync-sgd --learn-rate 0.0005 --seed 1111 ./build-hip/marian-decoder -m model.npz -v vocab.src.yml vocab.tgt.yml \ -i test.src -b 6 --devices 0 ``` Run on four Radeon Pro W7800 (gfx1100, ROCm 7.2.3). The log reports "Using NCCL 4.7.7 for GPU communication" and "NCCLCommunicators constructed successfully"; synchronous SGD over the four devices converges on a toy reverse-copy task (cost 0.76 at 1000 updates to 0.055 at 3000), and beam-search decoding of the resulting model reproduces all 30 held-out sentences exactly. Asynchronous (default) training over the same four devices was run as well and converges.
The convolution and pooling wrappers were cuDNN-only, so USE_CUDNN could not be turned on for AMD GPUs and the character-CNN encoder was unavailable there. MIOpen is ROCm's library of the same primitives; it now backs those wrappers. Read cudnn_wrappers.h first: the class declarations are shared and the handle and descriptor types are spelled through aliases, MIOpen describing convolution weights with an ordinary tensor descriptor rather than a separate filter type. The implementation in cudnn_wrappers.cu is then a self-contained block ahead of the cuDNN one, which is left exactly as it was. Three differences shape it rather than showing up call by call. MIOpen takes an explicit workspace and an algorithm chosen by benchmarking, so each convolution runs a find step, cached per input shape since a graph replays the same convolution once per batch; importantly the benchmark writes to whichever tensor it is given as the output, so the forward and backward finds are handed only destinations that are about to be overwritten. MIOpen's convolutions and bias addition honour only a unit alpha and a zero beta, so the accumulating form the backward pass needs is a convolution into a staging buffer followed by a tensor add. Its pooling backward pass reads indices recorded by the forward pass, so that workspace is a member of the wrapper. Two defects in the same code had to go with it, both of which also affect the CUDA build and are fixed for it too. Options never instantiated its accessor for a pair of ints, which is how the convolution layer reads kernel dimensions, paddings and strides, so nothing linked with the feature enabled. And PoolingOp kept the shape it inherited from its input instead of asking the wrapper for the pooled one, which for any window that actually shrinks the input left most of the result unwritten; ConvolutionOp already asks the equivalent question. Authored with the assistance of Claude, an AI assistant by Anthropic. Test Plan: ``` cmake -S . -B build-hip -G Ninja -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCOMPILE_CUDA=ON \ -DUSE_CUDNN=ON -DUSE_NCCL=ON -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON \ -DCMAKE_BUILD_TYPE=Release -DCOMPILE_TESTS=ON -DUSE_MKL=OFF \ -DUSE_TCMALLOC=OFF -DUSE_DOXYGEN=OFF -DBUILD_ARCH=native cmake --build build-hip -j ./build-hip/marian --type char-s2s -t train.src train.tgt -m model.npz \ --vocabs vocab.src.yml vocab.tgt.yml --dim-emb 64 --dim-rnn 64 --after 1000u \ --devices 0 --mini-batch 16 --char-highway 0 --seed 2222 ``` On a Radeon Pro W7800 (gfx1100, ROCm 7.2.3) a separate program built against the library checks convolution forward, its gradients with respect to input, kernel and bias, and max and average pooling forward and backward, all against references worked out by hand; a wider multi-output-channel convolution of the shape the character encoder uses is checked against a reference computed on the host in the same program. All agree. The character-CNN encoder then trains on GPU, with cost falling from 2.97 to 1.70 over 1000 updates on a toy task. The unit suites and the app tests are unaffected.
The AMD GPU build had no mention outside the CMake options themselves. The changelog now records it with the switches that turn it on and the ROCm libraries it builds against, and the feature list says which vendor's GPUs are supported through which toolkit. Build instructions themselves live on the project website rather than in this repository, so nothing is duplicated here. Authored with the assistance of Claude, an AI assistant by Anthropic. Test Plan: documentation only; no build or behaviour change.
The half-precision detour added for hipSPARSE broke the CUDA build on the
toolkits from 11.0 through 12.1.
The detour is selected by TypedSparseGemm::resultNeedsCast(), which is a
constant false on the CUDA path, so the block never runs there -- but a plain
`if` still instantiates it, and it contains `(ScalarType)0` with ScalarType =
__half. prod_sparse.cpp is a host translation unit (the legacy cuda_add_library
sends only .cu files to nvcc) and it instantiates TypedSparseGemm<half>
whenever CUDA is enabled, so that cast is compiled by the host compiler. CUDA
headers up to 12.1 keep __half's integer converts behind
`#if defined(__CUDACC__)`, leaving __half(float) and __half(double) equally
good matches for an int, and the compile fails with "call of overloaded
'__half(int)' is ambiguous". CUDA 12.2 changed that gate to
`#if !defined(__CUDA_FP16_DISABLE_IMPLICIT_INTEGER_CONVERTS_FOR_HOST_COMPILERS__)
|| defined(__CUDACC__)`, making the converts host-visible unless a build opts
out with that macro, which is why the break went unnoticed on a recent toolkit.
Both blocks are now `if constexpr`, so the CUDA build no longer instantiates a
branch that cannot run there, and the comparison uses a float literal. Behavior
on AMD GPUs is unchanged.
Written with Claude, the AI assistant.
Test Plan:
CUDA path, full compile and link with nvcc 12.8, no NVIDIA GPU present, all
161 targets and every executable link:
```
cmake -S . -B build-cuda -G Ninja -DCOMPILE_CUDA=ON -DUSE_HIP=OFF \
-DCUDA_TOOLKIT_ROOT_DIR=<cuda-12.8>/targets/x86_64-linux \
-DUSE_CUDNN=OFF -DUSE_NCCL=OFF -DUSE_FBGEMM=OFF -DCOMPILE_CPU=ON \
-DCMAKE_BUILD_TYPE=Release -DCOMPILE_TESTS=ON -DUSE_MKL=OFF \
-DCOMPILE_KEPLER=off -DCOMPILE_MAXWELL=off -DCOMPILE_PASCAL=off \
-DCOMPILE_VOLTA=off -DCOMPILE_TURING=off -DCOMPILE_AMPERE=on \
-DCOMPILE_AMPERE_RTX=off
cmake --build build-cuda -j
```
The exact prod_sparse.cpp host compile re-run against CUDA 12.1 headers, the
newest release that still hides the converts from the host compiler: it fails
at prod_sparse_cu11.h:191 with the ambiguity above before this change and
compiles clean after it. The same repro compiles against 12.2.53 and every
later release tried, which is how the boundary was located.
```
c++ -std=gnu++17 -DCUDA_FOUND -I<cuda-12.1>/include ... \
-c src/tensors/gpu/prod_sparse.cpp
```
The CUDA build is compile-checked and linked only; it was not run, as no
NVIDIA GPU is available here.
AMD GPU, Radeon Pro W7800 (gfx1100), ROCm 7.2.3, USE_CUDNN=ON USE_NCCL=ON:
```
cmake --build build-hip -j
./src/tests/units/run_operator_tests
```
operator 603/603 assertions (the sparse csr-dot sections included, fp32 and
fp16), graph 10/10, attention 6/6, transformer 3/3, binary 9/9, fastopt 23/23,
utils 8/8; rnn 21/24 with the three pre-existing random-number reference
mismatches.
MIOpen honours only a unit alpha and a zero beta for 2D pooling, so the accumulating backward pass is a pooling backward into a staging buffer followed by a tensor add. That buffer was a local, so every backward call allocated it and then freed it with a synchronizing hipFree. It is now a cached member grown through ensureBuffer, the same pattern the convolution gradients in this file already use, and PoolingWrapper already carried a member for the index workspace its backward pass reads. Per batch this saves nothing: the buffer is a shared_ptr with a hipFree deleter, the wrapper is a by-value member of the pooling node, and nodes are dropped when the graph is cleared for the next batch, so the allocation still happens once per pooling node per batch and the free moves from the end of the backward call to node teardown. The change is kept for consistency with the convolution-gradient path. Amortizing across batches would mean hanging the buffer off something that outlives a node. Written with Claude, the AI assistant. Test Plan: AMD GPU, Radeon Pro W7800 (gfx1100), ROCm 7.2.3, USE_CUDNN=ON: ``` cmake --build build-hip -j ./src/tests/units/run_operator_tests ./src/tests/units/run_graph_tests ``` The evidence for the changed code is a standalone program linked against the built library, which checks max and average pooling forward and backward against hand-computed references -- avg_pooling and max_pooling are the only callers of this wrapper, and in this tree only the MNIST LeNet example uses them. The same program checks convolution forward and its three gradients, plus a multi-output-channel convolution of the character encoder's shape: all agree. Character-CNN training converges as before over 1000 updates (cost 3.22 -> 1.70, no NaN). That covers the convolution wrapper and training as a whole; its pooling goes through pooling_with_masking, which has its own kernels and does not reach this file. ``` ./marian --type char-s2s --char-highway 0 --dim-emb 64 --dim-rnn 64 \ --mini-batch 16 --after 1000u --devices 0 \ --train-sets train.src train.tgt --vocabs vocab.src.yml vocab.tgt.yml ```
The MIOpen status check printed the raw enum value, while the cuDNN macro it mirrors prints cudnnGetErrorString. MIOpen has the same decoder, so a failure in the field can say what went wrong instead of leaving a number to look up. Written with Claude, the AI assistant. Test Plan: AMD GPU, Radeon Pro W7800 (gfx1100), ROCm 7.2.3, USE_CUDNN=ON USE_NCCL=ON: ``` cmake --build build-hip -j ./src/tests/units/run_operator_tests ./src/tests/units/run_graph_tests ``` operator 603/603 assertions, graph 10/10, attention 6/6, transformer 3/3, and the convolution and pooling reference checks all agree. The decoded string was confirmed by calling miopenGetErrorString on a failing status.
jeffdaily
marked this pull request as ready for review
August 12, 2026 17:13
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
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.
This adds AMD GPU support to Marian by porting the CUDA GPU backend to HIP, targeting ROCm. There is no AMD path in the tree today, so this is new hardware support rather than a change to existing behavior. The NVIDIA build is untouched except for three shared fixes called out in their own section below: everything else is additive or guarded, and with
USE_HIPoff the CUDA sources compile as before.An earlier revision of this branch left sparse matrix multiply, multi-GPU collectives and the cuDNN convolution path out of scope. All three are now ported and exercised on real GPUs, which is what most of the new work below is.
Build integration
A new
USE_HIPCMake option selects the HIP path. Marian compiles its GPU sources through the legacycuda_add_libraryfromFindCUDA, so underUSE_HIPthat call is satisfied by a shim macro that creates an ordinary library and marks the GPU translation units as HIP, drivingCMAKE_HIP_ARCHITECTURESinstead of a-gencodelist. No architecture is hardcoded; when the option is not set the architecture is auto-detected, and the build stops with a clear message rather than guessing if detection fails.Source changes
A single compat header aliases the CUDA runtime symbols Marian uses to their HIP equivalents. A small directory of forwarding headers (
cuda_fp16.h,cublas_v2.h,cublasLt.h,cusparse.h,curand.h,nccl.h,cooperative_groups.hand friends) sits on the include path for HIP builds only, so the sources keep their CUDA spelling and the diff stays small; on NVIDIA that directory is absent and the real toolkit headers are used.Library mappings are one-to-one: cuBLAS to hipBLAS, cuBLASLt to hipBLASLt, cuSPARSE to hipSPARSE, cuRAND to hipRAND, Thrust to rocThrust, cuDNN to MIOpen, NCCL to RCCL. The option defaults are upstream's, unchanged:
USE_NCCLis on and now works under HIP rather than having to be turned off, andUSE_CUDNNstays off by default but builds and runs when enabled.The one change worth close review
src/tensors/gpu/topk.cuandsrc/translator/nth_element.cureduce shared memory with a__syncthreads()tree down to 32 elements and then finish with an unrolled warp-synchronous tail that has no barriers and reads non-volatile shared memory. That is correct only where the hardware warp is 32 lanes wide and executes in lockstep. AMD CDNA parts have a 64-lane wavefront, so the tail's assumption does not hold there: the low 32 lanes are not implicitly synchronized across those steps and non-volatile shared memory may be register-cached, which can make the beam-search argmax silently wrong and non-deterministic. On the HIP path the tree now runs all the way down to a single element under__syncthreads(), preserving the original comparison order.Because this sits on the beam-search top-k, where a wrong result corrupts translations rather than crashing, it was checked with fixed-seed determinism runs (two runs bit-identical, and identical to a CPU decode of the same model) and not by inspection alone.
Sparse matrix multiply
CSRProdinsrc/tensors/gpu/prod_sparse_cu11.hnow produces correct results under hipSPARSE for both fp32 and fp16. The existingCUSPARSE_ORDER_ROWandCSR_ALG2selection is correct on hipSPARSE too and is unchanged; the two fixes below are independent of each other.For fp32, the multiply was never issued. The code called
cusparseSpMMonly insideif(bufferSize > 0); cuSPARSE always asks for scratch forCSR_ALG2so on NVIDIA the guard never fires, but hipSPARSE reports zero for these shapes andCkept whatever it already held. The allocation is now conditional and the multiply unconditional, which is the one behavioral difference this change makes on the NVIDIA path and is inert there for the reason just given.For fp16, hipSPARSE has no uniform half SpMM: half sparse and dense operands are accepted only against a float result matrix with float compute. The half instantiation therefore multiplies into a float scratch matrix and casts the result back, seeding that scratch from
Cwhen beta is non-zero so accumulation still means what it did. Scalars on this path are float to match the compute type. This costs a cast round-trip per call; it is correct but untuned, and worth revisiting if fp16 sparse attention throughput ever matters.Multi-GPU collectives
find_package(rccl)and anccl.hforwarding header are enough forsrc/training/communicator_nccl.hto compile unchanged, since RCCL keeps the NCCL API and symbol names. The vendored NCCLExternalProject(which does not build under hipcc) is skipped underUSE_HIP, and RCCL is linked from ROCm instead.This is exercised on four GPUs rather than compile-checked: synchronous SGD across
--devices 0 1 2 3converges on both a 4x MI250X and a 4x Radeon Pro W7800 host, the communicators construct, and decoding the resulting model reproduces the task exactly.Convolution and pooling
src/tensors/gpu/cudnn_wrappers.{h,cu}keep one set of class declarations, with the handle and descriptor types aliased to MIOpen's and the MIOpen implementation as a self-contained block ahead of the untouched cuDNN one. MIOpen uses an ordinary tensor descriptor for convolution weights rather than a distinct filter type, which the aliasing accounts for.Three MIOpen differences shaped the implementation. Workspaces are explicit and the algorithm is chosen by benchmarking, and the
miopenFindConvolution*Algorithmcalls write to whatever tensor is named as their output, so each search is given a destination that is about to be overwritten anyway rather than the live input or weight buffer; results are cached per input shape. Only alpha=1 and beta=0 are honored in 2D, so the accumulating backward form is a convolution into a staging buffer followed bymiopenOpTensor, and forward bias is likewise an add rather than the overwritingmiopenConvolutionForwardBias. Pooling backward reads the indices its forward recorded, so that workspace outlives the call.Three fixes the NVIDIA build needs too
These are the only changes here that alter CUDA behavior, and they are the reason
USE_CUDNN=ONworks at all.src/common/options.cppnever instantiatedGet<std::pair<int, int>>, which is howsrc/layers/convolution.cppreadskernel-dims,paddingsandstrides, so nothing linked withUSE_CUDNN=ONon any platform before this. The instantiation is added, next to the existing scalar ones;fastopt.cppalready had the matchingAs<std::pair<int, int>>and needs no change.PoolingOpinsrc/graph/node_operators_unary.hkept its input's shape instead of asking the wrapper for the pooled one, so any window that actually shrinks the input left most of the output unwritten. It now callsgetOutputShapethe wayConvolutionOpdoes.One caveat on the CUDA side:
cudnn_wrappers.cutargets the convolution algorithm-selection API that cuDNN 8 removed, so the CUDA cuDNN path does not compile against a current cuDNN with or without this change. The two fixes above are in shared files that are not cuDNN-specific, and they compile and link in the CUDA build.Validation
Linux, ROCm 7.2,
USE_HIP=ONwithUSE_CUDNN=ONandUSE_NCCL=ON, on AMD Instinct MI250X (gfx90a, 64-lane wavefront) and Radeon Pro W7800 (gfx1100, 32-lane wavefront), four GPUs of each. 294 of 294 targets build on both, and the numbers below are identical on the two architectures.operator_tests: 603 of 603 assertions, all four test cases pass, fp32 GPU, fp16 GPU and CPU. The sparse and fp16 affine sections are included; previously the sparse exception aborted the case and everything after it never ran on any platform.graph10/10,attention6/6,transformer3/3,binary9/9,fastopt23/23,utils8/8.rnn_tests21 of 24, the three unchanged reference-value mismatches described under limitations.poolingapp test only constructs a graph, so it is not evidence on its own.--type char-s2s), the only consumer of the convolution path, trains on GPU with cost falling steadily and no NaN or Inf. It needs--char-highway 0because the highway network callssigmoid(vector<Expr>), which aborts with "Not implemented" on every platform; that is unrelated to this change and not addressed here.USE_HIPoff.Windows, on Radeon 8060S (gfx1151) with a current ROCm toolchain: 277 of 277 targets build, the fp32 GPU unit suites pass (
graph10/10,attention2/2,transformer1/1,operator202/202 fp32 GPU and 198/198 CPU,binary9/9,fastopt23/23,utils8/8,test_prod), training runs on GPU, and beam-6 decode succeeds both on a toy model and on a production-size one (512-dimensional embeddings, six layers, 22k vocabulary, mini-batch 32). Radeon RX 9070 XT (gfx1201) passed the same checks on the earlier revision of this branch. MIOpen and RCCL are validated on Linux only.On Windows the fused GEMM-plus-bias path is computed as a plain GEMM followed by the existing broadcast bias add, under a
_WIN32guard, because the current hipBLASLt release faults inside algorithm selection for every such call there. Results are identical; this is the same fallback the pre-CUDA-11 path uses.Known limitations
src/common/types.hstubsfloat16out under_MSC_VER; the fp16 test sections abort there as they do on a Windows CUDA build. Not addressed here.rnn_testshas three failures in its "Simple RNN" case on every AMD architecture. The test compares against reference values hard-coded from cuRAND's generator; hipRAND produces a different, equally valid random stream, so the initial weights differ. It is a test-fixture assumption, not a computation error.topk.cuis not ported; on HIPSort()uses the rocThrust sort-by-key path instead. Only the tests reach it.Build recipe
CMAKE_HIP_ARCHITECTUREScan be set to target a specific GPU; left unset, it is auto-detected from the host.This pull request was prepared with the help of an AI assistant (Claude) and was read and approved by a person before it was opened. It comes from an ongoing effort to add AMD GPU support to widely used CUDA projects, one repository at a time: https://github.com/AMD-Ecosystem/moat -- that repository describes how the work is done and what a person checks before anything is submitted.
If you would rather not receive pull requests from this effort, say so here or open an issue at https://github.com/AMD-Ecosystem/moat/issues/new/choose and we will close this and stop. That can cover this repository alone or everything you own, whichever you prefer.