Repository navigation
Add native AMD HIP/ROCm Tensor operators - #3809
Conversation
MrNeRF
left a comment
There was a problem hiding this comment.
Built it with the pinned SDK 10.1.0:
- Builds: gfx1150 and gfx942 build in under a minute each. The full 26-target compressed module builds with the CI job's exact flags.
- Tests: all CPU CTest groups pass (5/5 for each build), and the new Python tests pass.
- ABI revision: revision 3 rejects real revision 1 and 2 modules, not just the mock.
- Contracts: from reading, most operators match the CUDA backend: sort NaN/±0 ordering and stability, argmin/argmax, conversions, index clamping, index_add duplicates, cdist p=0/inf and empty axes.
- CI filter: it works on real history and leaves the other jobs alone.
- The 26-target compile will likely exceed the job's timeout.
- It took 906 s here, against 26 s for #3782. Device compile and link run per target inside one clang call, so
--paralleldoesn't help. - #3782's step ran about 1.6x slower on GitHub than here. That puts this one at about 25 minutes, against the job's
timeout-minutes: 20(ubuntu.yml:47). - The PR's own run was cancelled before it started, so this hasn't been tried on CI.
- Could you cut the instantiations (see 5), parallelise the offload compile/link, or raise the timeout?
- It took 906 s here, against 26 s for #3782. Device compile and link run per target inside one clang call, so
- Byte predicates compare the scalar as a float,
float(a) < scalar(tensor_ops_runtime.hpp:822–831). CUDA casts the scalar to the element type instead (scalar_right_op, tensor_functors.hpp:885–888).- For a UInt8 tensor, x=2 < 2.5 is false on CUDA and true here.
- x=0 == 0.5 is true on CUDA and false here.
predicate_typedalready does the cast for I32/I64/U32. Routing UInt8/Bool through it fixes this and dropsboolean_pointwise(~55 lines).
- Reduction op codes mean two different things within the same ABI.
lfs_hip_reduce_typed_layoutuses the coreReduceOpnumbering, 2=Max and 3=Min (integer_reduce_runtime.hpp:61, 111–112).lfs_hip_reduce_f32,reduce_layout_f32andchain_reduce_f32useLfsHipReduceOp, where MIN=2 and MAX=3 (runtime_api.h:149–152).- integer_reduce_api.h names no enum, so an adapter passing
LFS_HIP_REDUCE_MAXto the typed reduction gets min. - Could both use one enum?
- masked_select always stages its result in a full-size workspace that stays allocated.
select_stableonly writes directly when capacity >= count (tensor_ops_runtime.hpp:768–781), but the frontend passes the exact count (tensor_masking_ops.cpp:273–295).- A 100M-element float masked_select therefore keeps ~400 MB until
trim_workspace. CUDA writes straight into the result.
- Size.
- There are ~3,270 implementation lines. A single-target module grew from 61 KB to 11 MB with 3,045 device functions. hipcub/rocprim make up 56% of that: 144 segmented radix sort kernels, and 1,040 DeviceSelect kernels from 11 type combinations.
- About 300–350 lines repeat logic that already exists:
integer_math_nativeduplicates the shared div/mod/pow/min/max/abs/neg/relu/sign functors that you already compile in, andclamp_i32duplicatesclamp_range_op.- The four block-reduction kernel families, with copied parts-splitting and kept/reduced lambdas, could be one.
- 17 overlap lambdas and 6 copies of the lazy workspace allocation could each be one helper.
lfs_hip_broadcast_f32andlfs_hip_fill_strided_f32are aliases ofbroadcast_typedandfill_layout_bits.
- A single
DeviceSelect::Flaggedper element width, instead of 11 type combinations, would also shrink the binary and help with 1.
Smaller things:
- ReLU(-0) gives -0 through the generated functor (prepare_pointwise.py:26–30). It gives +0 on CUDA and in your own GEMM bias+ReLU (gemm_runtime.hpp:19, 57). Worth picking one.
- There are 44 narrowing warnings under
-Wconversion, for example dim3 in gemm_runtime.hpp:94–111 and tensor_ops_runtime.hpp:1507/1509. They're harmless given the validation but easy to clean up.
|
Addressed the review in
The two smaller comments are addressed: HIP device ReLU(-0) now produces +0 consistently with fused GEMM/ReLU, and host/device Validation at the updated head
The PR description now records these results while retaining the earlier validation as historical context. Linux checks remain compilation/CPU coverage, not GPU execution on unavailable devices. This still does not enable the application Tensor adapter or training. |
MrNeRF
left a comment
There was a problem hiding this comment.
Rebuilt it at df889f4 with SDK 10.1.0: gfx1150, gfx942, and the 26-target compressed module with the CI flags. This box has no AMD GPU, so the GPU test groups didn't run.
What's fixed:
- CI time: the job allows 60 minutes. The 26-target build took 829 s here (~22 min at GitHub's speed). The module shrank from 6.1 to 4.8 MB, and gfx1150 from 11.0 to 8.0 MB.
- Reductions: one enum. The real ABI 3 module is rejected with "expected 4, got 3" before any export is bound.
- masked_select: no full-size staging buffer anymore.
- ReLU(-0): gives +0 everywhere.
-Wconversiondrops from 48 warnings to 0 (host and device). - Consolidation: shared integer functors,
clamp_range_op, one block-reduction kernel and one workspace helper. The accumulation order is unchanged in every path, from reading. - Tests: CPU CTest 5/5 on all three builds, Python 26/26.
A correction to my round-1 point 2: it was wrong. Through the Tensor API, a UInt8 tensor compared with a float scalar is promoted to Float32 before any backend sees it (promote_dtypes in the scalar comparison ops). So on CUDA, 2 < 2.5 is true, and the backend's cast is never reached. Your change is harmless; sorry for the noise.
What I'd still change:
- Bool now has two rules.
predicate_typedwith Bool converts the scalar withbool(scalar), so 0.5 becomes true (predicate_runtime.hpp:62–70).lfs_hip_boolean_pointwisepasses Bool data as U8 (tensor_ops_runtime.hpp:924), so 0.5 becomes 0.- The test pins the first rule, but #3759's adapter used
boolean_pointwise. - Deleting
boolean_pointwiseleaves one rule.
- The selection kernels grew 2–3.3x. DeviceSelect went from 177 to 275 KB per target.
- Values are read through a counting iterator plus memcpy, and the mask through a per-element switch over six dtypes, although value selection always has a UInt8 mask (tensor_ops_runtime.hpp:806–896).
- Plain pointers per width, plus one generic instantiation for nonzero positions only, would fix it.
selection_count(:832–847) is the same kernel ascount_nonzero_kernel(:119–140).
- Extra passes and host syncs that CUDA doesn't do. The last one matters most for training loops.
- masked_select now costs 3 mask passes and 3 syncs, against 2 and 1 on CUDA. The frontend already counted and passes the exact count, and CUDA trusts it (cuda_ops_index_mask.cpp:288–301).
- masked_scatter rescans and syncs to recheck
selected_count(:1139–1155). check_indicesruns a validation kernel and a blocking read before every checked gather, index_select, index_fill, index_add and index_cast (:619–631). CUDA records index faults asynchronously instead.
- What can go. Nothing below is reachable from the Tensor API, and CUDA has no counterpart either:
- Exports without a caller, or that only forward:
broadcast_f32,fill_strided_f32andboolean_pointwise;scalar_left_typed: nothing insrc/builds a scalar-left program, and it is 28 kernels per target;where_scalar;- the
workspace_growth_bytesout-parameter on arg-extreme.memory_infoalready reports it.
- Instantiations:
- the generator emits all 68 ops for Float16, but the Tensor API reaches 17 of them (prepare_pointwise.py:74–78);
- 42 conversion pairs, where CUDA serves 24.
- Together that's about 150 kernels per target.
- Duplicates:
- strided copy and strided scatter are special cases of
copy_layouts; validate_indicesand the inline check inindex_castrepeatcheck_indices;index_fill_kernelisindex_update_kernelwith a constant source;- the checked shape product is written 4 times;
- 12 hand-written overlap checks remain next to the new helpers;
- the comparison switch in integral_broadcast_runtime.hpp:15–28 repeats predicate_runtime.hpp:11–30.
- strided copy and strided scatter are special cases of
- Generated code:
hip_pointwise_functors.hppis a 1,592-line text-patched copy of the shared functors. A device-guard macro in the shared header would let HIP include it directly. The generator'sSCALARS/COMPOSITEStables also restate pointwise_lowering.hpp. - README: about 35 of the 95 lines narrate code or review history: the block kernel, the enum, DeviceSelect, the generator and the GPU tests. The support table and the contract paragraphs are what readers need.
- Tests: the two sort tests have the same body. Five reduction tests and five byte-predicate tests overlap heavily.
- Total: that's roughly 250 implementation lines, 70 test lines and 35 README lines.
- Exports without a caller, or that only forward:
- CI time:
- The 60-minute limit hides that everything is one compilation unit: runtime.hip includes the nine
*_runtime.hppfragments, so the 26-target build is one serial clang call of ~14 minutes, and any edit rebuilds all of it. - Adding
--offload-jobs=2cut it from 829 s to 600 s here (one CMake line). - Splitting the runtime into 3–4
.hipfiles would fix it properly.
- The 60-minute limit hides that everything is one compilation unit: runtime.hip includes the nine
Smaller things:
- The ABI style is mixed.
- Some calls take versioned descriptors, while gather and index_update take 11–12 positional parameters.
LfsHipPredicatedoubles as the integer-arithmetic descriptor, and itsreservedfield means "scalar on the left".LFS_HIP_REDUCTION_WORKSPACE_FLOATSis part of the ABI.
- CI: the HIP compile job of run 40216155252 has been queued since 18:33 UTC and hasn't started yet.
|
Follow-up to the latest review: pushed the revisions through Operator surface and kernel instantiations
I retained Selection and indexing
The small asynchronous error flag uses owned pinned host staging and a copy queued on the producer stream. This follows a reproducible SDK/driver observation on the tested Windows system where a blocking four-byte flag copy did not observe the value seen by the queued copy. The completed fence leaves no reset queued behind it: reset is deferred until the next checked submission. A fault-injection test verifies that distinction. ABI and build organization
Validation
For the measured Linux compilation configuration, the 26-target build decreased from 9m28.477s to 4m23.69s. This combines the reduced instantiations, translation-unit split and offload parallelism; it is a compilation measurement, not a training-performance claim. The final small fence refinement was subsequently rebuilt and tested on Linux as well. These results are local validation; GitHub CI for the newly pushed revision still needs to complete. |
MrNeRF
left a comment
There was a problem hiding this comment.
Rebuilt it with SDK 10.1.0 (gfx1150, gfx942 and the 26-target compressed module); no AMD GPU here, so the GPU test groups didn't run.
| Round 2 | Now | |
|---|---|---|
| 26-target compressed build | 829 s | 533 s |
| 26-target module | 4.78 MB | 4.56 MB |
| gfx1150 module | 7.98 MB | 7.56 MB |
| gfx1150 DeviceSelect code | 275 KB | 120 KB |
| gfx1150 kernels (> 8 B) | 524 | 429 |
| CPU CTest (each build) / Python | 5/5 / 26/26 | 5/5 / 26/26 |
What's fixed:
- One Bool rule:
boolean_pointwiseis gone. - Selection: it reads plain pointers per width, and the duplicate count kernel is gone.
- No extra host reads: precounted
masked_select,masked_scatterchecks the count on the device, and index checks run inside the kernels. - Instantiations: 17 FP16 operations, the 24 CUDA conversion pairs, 4 scalar-left operations.
where_scalarand the four scalar-left ops are right to keep, since the adapter calls them. - Shared functor header: it's now included instead of copied.
- ABI 5: a real round-2 module is rejected with "expected 5, got 4" before any export is bound.
- Build: the three translation units plus
--offload-jobs=2build in ~14 min at GitHub speed, well inside 60.
One thing before it can go in:
A staged readback can return values from an invalid index op as a success. Checked indexing now returns right after submission and leaves its error in the context flag. But lfs_hip_readback_wait (runtime.hip:295) only waits for the readback event and checks the copy error, and lfs_hip_readback_poll (:314) copies the staged bytes and sets ready=1 without looking at that flag. So invalid gather → readback → wait → poll hands back stale values from the skipped lanes as a good result, and the -2 only shows up on a later context sync. Before, gather rejected the indices before returning.
I reproduced it with a host-side probe that runs your actual host functions with simulated queues: wait=0, poll=0, ready=1, value=99, pending_error=1. Please carry the pending execution error through wait/poll (and the packed readback) before exposing the result, without attributing a later operation's error to an earlier snapshot. A regression for invalid indexing followed directly by a readback would cover it.
Smaller:
fillandaffine(runtime_common.hpp:201) are compiled into all three objects, andarange_kernel(pointwise_runtime.hpp:48) into two. Defining each once saves ~1.3 KB per target.- The PR description still says 68 FP16 operations, a copied functor header and ABI 4. It should also describe when deferred index errors surface and that valid lanes are written even when others are invalid.
|
Addressed the remaining review requests in 9abcf08. Deferred errors belong to the readback snapshotThe reported failure was reproducible on the previous module: four new regression methods failed, with five failed assertions including the empty-readback/marker variants. They pass after this correction.
Ordinary readback/markers add no kernel, device allocation or host wait for error capture: only an asynchronous flag copy when checked work is pending. Packed accumulation uses its own small device flag, which is included in memory accounting. Regressions cover invalid-index → readback → wait/poll, direct polling without a preceding wait, earlier-valid/later-invalid snapshots, context-error consumption, packed appends across reset, empty readbacks, ordering markers, masked-scatter cardinality errors, failed-result discard/reuse, and fault-injected ownership/recovery. Valid lanes may still have written before an index fault; this remains an asynchronous error contract, not transactional rollback. Duplicate kernels and description
The PR description now reflects 17 reachable FP16 operations, 24 conversion pairs, direct inclusion of shared functors, ABI 5 and the deferred-error/readback contract. Earlier measured build times, sizes and numerical comparisons remain explicitly marked as historical validation. The CI selector also now watches ABI remains 5: this correction changes opaque internal state and error propagation, without changing exported signatures or descriptor layouts. Verification
No new training-quality/performance, macOS runtime or CUDA GPU result is claimed. Remote CI for the new head still needs to complete. |
MrNeRF
left a comment
There was a problem hiding this comment.
The readback fix works. I reran my host-side probe against the new functions: the invalid-index case that used to come back as wait=0, poll=0, ready=1, value=99 now gives wait=-2, poll=-2, ready=0 and leaves the destination untouched. The extended probe passes 44 of 44 cases (10 of 44 on the previous head), including 192 slot and stream reuse cycles. Those cover an earlier valid snapshot followed by a later invalid op, packed appends across a reset and after the context error was consumed, empty readbacks, ordering markers, and discard and reuse after a failed poll. It runs clean under ASan/UBSan, and no pinned, device or event resources leak. fill/affine and arange_kernel are each defined once now, and a launch error still reaches the caller with its native code. The CI selector picks up pointwise_lowering.hpp changes, and since the private API headers are unchanged apart from comments, staying on ABI 5 is right.
Rebuilt with SDK 10.1.0 from a clean export of this head: gfx1150 7.55 MB, the 26-target compressed module 4.55 MB, CPU CTest 5/5 on both, Python 26/26. There's still no AMD GPU here, so the GPU groups didn't run on my side.
Small thing, no change needed: the extra hipGetLastError() in the empty chain reduction is redundant but harmless.
|
Removed the redundant Both Windows SDK 10.1.0/gfx1150 modules were rebuilt incrementally; 9/9 CTest groups passed, including empty sum/mean chain reductions and fault recovery. clang-format, whitespace and the error-debt gate passed. No ABI or descriptor change; no new Linux/macOS or training-performance result is claimed for this cleanup. |
Purpose
This is the fourth part of the AMD HIP/ROCm work originally proposed in #3759, following the shared Tensor contracts (#3776), native runtime foundation (#3781), and owned asynchronous transfers (#3782).
It adds native Tensor operators behind the private HIP runtime boundary. The implementations use context-owned buffers and streams, validate descriptors before launching work, and report errors through the existing C ABI.
HIP is still not selectable in the application, and this PR does not enable HIP training. The public Tensor adapter, backend registration, image preparation, viewer integration and distribution remain separate work. No CUDA, Metal or Vulkan implementation or trainer is changed.
Native operations
The developer README documents the storage subsets accepted by each family. Unsupported combinations fail explicitly; no implicit CPU fallback is introduced. Floating reductions may differ in rounding order between devices, and reproducible seeds do not imply identical RNG sequences across different backends.
Ownership, errors and edge cases
Views retain the context-owned buffer handle and byte offset. Operations validate ownership, ranges, alignment, layout spans, shape compatibility and their alias restrictions. Descriptor and range validation precedes submission. Checked indexing validates each lane in the kernel and skips out-of-range accesses; valid lanes may still be written, so this is not transactional rollback. Batched layout-copy descriptors are all validated before any copy is submitted. Exact in-place operations are accepted only where their contract permits them.
Reduction, scan and sort workspaces belong to the context. Growth drains previous users and releases disposable storage before allocating its replacement. Failed release retains ownership for retry; trim and context destruction release all workspace categories. Host scalar readback drains the stream before a blocking copy, so a failed drain cannot leave a queued write into expired stack storage.
The private ABI is revision 5. All reduction entry points use
LfsHipReduceOp, including typed axis reductions. Loaders check the exact revision before binding exports; revisions 1 through 4 are rejected. The readback correction changes opaque internal state and error propagation, not exported signatures, so it does not require another revision.The local correctness review also added regressions for capped launch grids above 1,048,576 elements and shifted byte-predicate aliases. Those cases failed against the preceding implementation and pass after the corresponding corrections.
Maintained tests and CI
The SDK-only tests compare native results with independent small CPU/scalar references. They cover conversions, large integers, NaN/Inf/signed zero, stable ordering, empty axes, noncontiguous layouts, partial launch blocks, invalid descriptors and output preservation. Matrix-distance tests cover short and tiled paths and multiple norms; indexed addition tests include duplicate destinations.
The separate instrumented module exercises allocation/free and stream-drain failures, workspace growth/trim, context cleanup retries and balanced device/pinned/event resource counters. Fault-injection entry points remain absent from the production module.
CPU-only module checks bind the native exports without querying a GPU. Generator regressions run without an SDK and join the existing Python fast suite. The path-gated HIP compile job inherited from #3782 additionally watches the shared pointwise catalog, functors and generator tests, so changes to those inputs cannot silently bypass native compilation. Workflow triggers and non-HIP jobs remain unchanged.
Deferred errors and readback snapshots
Index and masked-scatter cardinality errors surface at context synchronization, blocking host readback, or staged readback wait/poll. A staged readback captures preceding execution errors at its own queue position before its completion event. Later operations cannot invalidate an earlier snapshot. Packed appends accumulate their captured errors, including across context-error consumption/reset. Wait/poll report a failed snapshot before exposing bytes or successful readiness; a drained null poll discards it while returning its error. Context synchronization consumes its own pending error independently and cannot validate an already failed snapshot.
Ordinary readback and ordering markers need no additional kernel: the error flag is copied on the producer stream into slot-owned pinned storage. Packed accumulation uses a slot-owned four-byte device flag. Both remain owned across capture, synchronization and cleanup failures. Shared fill, affine and arange kernels now each have one definition.
Latest local validation
At
9abcf088f:b28947e0cmodule and reproduced the missing readback error (five failed assertions including the marker variants). They pass with the correction.pointwise_lowering.hpp, an input read by the pointwise generator.The preceding
b28947e0crevision also compiled the 26-target compressed module locally in 4m23.69s, compared with 9m28.477s for the earlier configuration below. That combined reduced instantiations, three translation units and--offload-jobs=2; it is a compilation comparison, not a training-performance claim. The final readback revision was rebuilt for all targets without a new controlled timing comparison.Earlier review validation at df889f4 (retained)
At
df889f403, includinga7c7c11bb:-Wconversionwith no warnings, whitespace, error-debt baseline and actionlint passed.The HIP-only CI job timeout is now 60 minutes. DeviceSelect is instantiated by four storage widths, and ascending/descending sorting share the same radix-sort direction. No non-HIP job is changed.
Earlier validation (retained for reference)
After rebasing onto
devata64dfee72, including the merged #3782 follow-up:These checks qualify the private native operator boundary. They do not establish full application integration, training quality/performance, physical device-loss recovery, Linux GPU execution, or runtime correctness on unavailable hardware, including wave64 devices. No new macOS execution is claimed for the final rebase; HIP compilation remains unsupported there.
Only implementation, maintained tests and build/capability documentation are committed. Local reports, benchmark experiments and datasets are excluded.