Repository navigation
perf(hipdnn): remove per-element heap allocation from CPU references - #11931
Merged
Merged
Conversation
…ner loops CpuFpReferenceConvolution heap-allocated a std::vector<int64_t> at every level of its nest: one per output element for the parallel functor's index vector and the spatial-index copy, one per (output element x channel) for the kernel iteration, and three per MAC for the x spatial indices and the two buildTensorIndices calls. ResNeXt2Group forward (N=8, C=64, 28x28, K=128, G=2, 3x3) issued roughly 7.2e8 allocations for one test case, from hardware_concurrency() threads at once. Runtime tracked allocation count rather than arithmetic - Grouped8MidRes has half the MACs of ResNeXt2Group, the same output-element count, and cost the same - and the Windows heap serialised what glibc's per-thread cache absorbed, leaving conv 22-35x slower on Windows than Linux while layernorm and pointwise sat at the ordinary 1.3-1.6x. Hoist the raw pointers and strides once and index flat offsets in the inner loops, as CpuFpReferenceMoeGroupedMatmul already does. Each spatial dimension maps its window index to a source index independently, and independently decides the tap has no source element - it lands in padding, or for dgrad is not stride-aligned - so validity and both offsets factor per dimension. The new detail::ConvolutionWindow resolves the whole window once per output element into (windowOffset, sourceOffset) pairs instead of re-deriving it for every (output element x channel x tap), and reuses its buffers so a thread_local instance stops allocating after the first element. ParallelTensorFunctorDynamic now fills one index buffer per thread instead of returning a fresh vector per work item, which helps every CPU reference rather than just convolution. Taps are emitted in row-major window order, the order iterateAlongDimensions accumulated in, so results are bit-identical rather than merely within tolerance. Verified by checksumming fprop/dgrad/wgrad output against the previous implementation across NCHW/NHWC/KYXC, 2D and 3D, grouped, strided, dilated and asymmetrically padded cases. Flat stride arithmetic bypasses getIndexImpl, which RaggedTensorBase overrides to rebase each batch at ragged_offset[b], so validateInput now rejects ragged tensors - the same guard the MoE references use. No convolution path passes one today. Measured on Windows / gfx1151. Standard/*Conv* in hipdnn_gpu_ref_tests (342 cases, CPU reference against the GPU reference) drops from 40.50 s to 8.36 s; the residual is GPU-side work this change does not touch. Calling the reference directly, ResNeXt2Group goes from 3.477 s to 0.028 s forward, 3.617 s to 0.015 s dgrad, and 3.876 s to 0.014 s wgrad. Layernorm and RMSNorm still use the slow path. Tests: hipdnn_test_sdk_tests (2073 passed); hipdnn_gpu_ref_tests Standard/*Conv*:Full/*Conv* (756 passed).
…g per element Same defect as the convolution reference, one level less severe. Both normalization references built a fresh std::vector<int64_t> for every element they touched - layernorm in buildFullIndices, RMSNorm in the fullIdx/scaleIdx/invRmsIdx copies - and handed it to getHostValue/setHostValue, which costs a virtual memory() call plus a stride reduction on top of the allocation. Layernorm bprop pays that three times per element across its passes. Layernorm was already the largest single line item in the Linux gpu-ref job at 266 s, and 430 s on Windows. The walk is the same for every output position, so hoist it: detail::buildDenseOffsets builds the flat offsets of one dense sub-block walk once per call, and the inner loops index base[positionOffset + table[i]] through raw pointers. detail::flatOffset covers the scalar side - the position that is held fixed while the other index space is walked. Both are shared with future conversions. The two references need mirrored tables because their parallel decomposition inverts. Layernorm fprop and bprop pass 1 fix a batch position and walk the normalized dims; bprop pass 2 fixes a normalized position and walks the batch dims. RMSNorm forward and the data-gradient pass fix a leading position and walk the reduction dims; the weight/bias pass does the reverse. Tensors addressed by the composed index (x/y/dy/dx) get a walk-aligned stride vector that zeroes the axes the walk does not reach; tensors addressed by only one side (scale/bias/dscale/dbias, mean/rstd/invRms) use their own strides directly. Walks stay row-major, which is what iterateAlongDimensions produced, so results are bit-identical rather than merely within tolerance. Verified by checksumming every output of both references against the previous implementation over the Medium4D/5D and parked Large catalog shapes in NCHW/NHWC/NCDHW/NDHWC, including the layernorm bprop path where mean and rstd are absent and the Welford recompute plus tmpMean scratch indexing runs. Ragged tensors are now rejected in both references, as in convolution and the MoE references: flat stride arithmetic bypasses getIndexImpl, which RaggedTensorBase overrides to rebase each batch at ragged_offset[b]. The graph executor already rejects them one level up; this covers direct callers. Also fixes a latent out-of-bounds read in layernorm bprop: when batchDims is empty the code appended a scalar batch dim but left batchStrides empty, then indexed batchStrides across the whole of batchIndices. batchStrides is now resized to match. Layernorm's private buildFullIndices helper is removed - nothing calls it now. Measured on Windows / gfx1151, calling the references directly. Layernorm 32x32x32x28x62 (parked Medium5D -> Large5D): fprop 0.618 s -> 0.034 s, bprop 1.297 s -> 0.126 s, bprop without mean/rstd 1.618 s -> 0.128 s. RMSNorm 16x64x225x225 (parked Medium4D -> Large4D): forward 1.519 s -> 0.044 s, backward 2.401 s -> 0.071 s. In hipdnn_gpu_ref_tests the 816 Quick+Standard layernorm and RMSNorm cases drop from 182.8 s to 145.3 s; the remainder is GPU-side reference execution and tensor setup, which this does not touch. Tests: hipdnn_test_sdk_tests (2073 passed); hipdnn_gpu_ref_tests Quick/*:Standard/* (3846 passed, 202.4 s -> 175.4 s).
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #11931 +/- ##
===========================================
- Coverage 70.43% 69.43% -1.00%
===========================================
Files 2933 2833 -100
Lines 482478 471637 -10841
Branches 69519 69571 +52
===========================================
- Hits 339816 327462 -12354
- Misses 118813 120432 +1619
+ Partials 23849 23743 -106
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
adickin-amd
commented
Sep 10, 2026
The convolution reference kept its ConvolutionWindow in a function-local thread_local because ParallelTensorFunctorDynamic invokes its callee with indices only. A callee needing state that outlives a single work item - a buffer it rebuilds per output element but must not reallocate per output element - had nowhere else to put it. The functor's own index buffer does not have this problem precisely because it lives in the thread body, which is the per-thread scope; the reference's window lives one level down, inside a callee the scheduler treats as opaque. Add ParallelTensorFunctorWithScratch<Scratch, F> and makeParallelTensorFunctorWithScratch<Scratch>(func, dims) alongside the existing factory. It constructs one Scratch per worker thread, reuses it for every work item that thread handles, and invokes the callee as func(scratch, indices). The lifetime is now explicit and ends with the parallel region instead of being implied by thread-local storage. The work split and index decomposition move to a new ParallelTensorRange, which both functors inherit, so they differ only in what they hand the callee. Inheritance rather than composition because the existing unit tests read `functor.totalElements` and `functor.strides[0]` directly, and getNdIndices is part of the surface those tests exercise. A distinct factory name rather than an overload of makeParallelTensorFunctor: an overload taking an explicit leading Scratch template argument does resolve, but only because the one-parameter form becomes non-viable through a substitution failure on the parameter type. That is too subtle for a header fifteen references depend on. Convolution's fprop, dgrad and wgrad take the window as a parameter and drop their thread_locals. No arithmetic changes; the diff is three lines per function. One behavioral narrowing: runChunked captures the body by reference rather than copying *this* into every thread, so the callee is now shared across threads instead of copied per thread. This was already effectively the case - the thread lambda was non-mutable and operator() is const, so no callee could ever mutate its by-value captures - and it removes a per-thread copy of the callee plus both stride vectors. Threads are joined before runChunked returns, which is what makes the by-reference capture safe; that guarantee is documented on the method. Tests. TestCpuFpReferenceRaggedRejection.cpp is new and covers the direct callers of the convolution, layernorm and RMSNorm references, which the graph executor's rejection does not sit in front of: the samples and the gpu-ref fixtures. It asserts the rejection is for raggedness rather than for some unrelated shape check that throws from the same function. TestCpuFpReferenceUtilities.cpp gains coverage for buildDenseOffsets, flatOffset, ConvolutionWindow and the new scratch functor, including the per-dimension validity factoring the window's correctness rests on and the per-thread-not-per-item scratch lifetime. Both groups were mutation-checked rather than merely observed to pass. Neutering validateNoRaggedTensor fails all eight rejection tests; two of them fail by crashing the process, because a ragged output tensor's buffer is ragged_offset[B] elements long while flat stride arithmetic over the padded dims addresses past its end. The guard prevents an out-of-bounds write, not just wrong numbers. Moving the scratch construction inside the work loop fails exactly the two tests that pin that contract. Tests: hipdnn_test_sdk_tests (2097 passed, 3 skipped, 24 new); hipdnn_gpu_ref_tests Standard/*Conv* (342 passed, 7.3 s versus 8.4 s with the thread_local).
Hoisting raw base pointers past TensorBase::getIndex also skipped its argument-count guard, which used to reject rank-mismatched inputs on every access. Nothing replaced it, so three reachable cases went from a named exception to out-of-bounds access. RMSNorm backward is the serious one: it validated dy, scale and invRms against x but never dx, dscale or dbias, which are then indexed at strides().data() + reductionStart. A short stride vector is read past its end while computing the offset - so the call returned success with plausible but wrong dscale/dbias and no diagnostic. Layernorm never checked normalizedDimCount against scale.dims().size(), only against ndim. A scale rank below normalizedDimCount underflows normSuffixStart = normIndexCount - normalizedDimCount and writes the walk strides far out of bounds, in both fprop and bprop. A rank-0 mean or rstd is a second instance: the scalar-batch padding makes the walk one index deep while the tensor's stride vector stays empty. Validate the ranks up front in each entry point, matching the convention CpuFpReferenceRMSNorm.hpp already had for scale. A rank mismatch is a caller error and keeps throwing; nothing is clamped. The checks are off the hot path and restore the diagnostic the hoist removed. These are reachable only by calling the references directly, which the samples, the gpu-ref fixtures and the unit tests all do - the graph path rejects the shapes earlier in the frontend. That bounds the blast radius but does not fix the oracle, which is what these references are. Found in review by a second pair of eyes, who reproduced each case under ASAN against both sides of the change. Tests: TestCpuFpReferenceRankValidation, 8 cases, each mutation-checked by removing its guard. hipdnn_test_sdk_tests (2107 passed, 3 skipped).
The early-exit trait probed is_invocable_r_v<bool, F, std::vector<int64_t>> - an rvalue - while the call site now passes the reused index buffer, an lvalue. A bool-returning functor taking std::vector<int64_t>& used to fail to compile against the rvalue call; it now binds fine at the call, but the trait still reports not-invocable, so the void branch runs and the bool is silently discarded. The functor visits its whole range instead of stopping. Probe with std::vector<int64_t>& - the exact type of the call-site expression. Note that probing const& (the obvious fix) is not enough: const& will not bind to a std::vector<int64_t>& parameter, so that functor would still take the void branch. The non-const lvalue probe matches by-value, const& and & parameters alike. Same correction for the scratch functor's two-argument probe. No in-tree caller takes a mutable index reference today, so this is latent. Tests: two cases, one per functor, using a bool-returning functor with a non-const lvalue parameter. Both fail against the rvalue probe (visiting 100 elements instead of stopping at 4).
…tics Two paths that are covered separately but never together: an empty batch index space (every input dimension is normalized, so the scalar-batch padding runs) and absent mean/rstd (so bprop recomputes them with Welford into the tmpMean/tmpRstd scratch, indexed through batchStrides). Their intersection is exactly what the batchStrides resize addresses. batchStrides is generated before the padding, so it is empty there while the walk is one index deep; reducing the padded index against it read past the end of the vector. The previous implementation segfaults on this input, which is also why the equivalence harness used to validate that change could not have covered it - there was no working baseline to diff against. Runs the recomputed-statistics path against the supplied-statistics path rather than against hand-computed values, since the contract is that the two agree. Asserts a non-zero result so a uniformly zero output cannot satisfy the comparison vacuously. Removing the resize fails this test. Gap identified in review.
getNdIndices had no callers outside its own unit tests once the parallel functors switched to filling a reused buffer. Delete it and point the tests at fillNdIndices, which is what they were really exercising. ConvolutionWindow's thread-safety and iterator-invalidation requirements appeared only as a performance aside about thread_local, so someone reaching for a static would get a silent data race and a wrong oracle. State them as contracts instead, and point at ParallelTensorFunctorWithScratch as the way to obtain an instance. Comments that narrated the removed implementation now describe the code as it stands. The row-major tap ordering stays documented - it is part of the numerical contract, not an implementation detail - but without the back-reference to what it used to match. Raised in review. Tests: hipdnn_test_sdk_tests (2108 passed, 3 skipped).
The convolution reference was formatted with clang-format 20, while .pre-commit-config.yaml pins mirrors-clang-format v18.1.4. The two disagree on how to break a multi-argument call whose last argument is a lambda: 18 breaks after the open paren and indents the arguments, 20 aligns them to the paren. Reformatted with the pinned version, which is what the gate runs. Whitespace only.
|
🎉 All checks passed! This PR is ready for review. |
adickin-amd
marked this pull request as ready for review
September 10, 2026 21:13
modernize-return-braced-init-list, which the hipDNN tidy configuration runs with WarningsAsErrors, so it failed the superbuild lane. The Linux clang-tidy workflow passed because it does not cover this target; the superbuild lane is what caught it.
EwanC
self-requested a review
September 15, 2026 08:41
EwanC
approved these changes
Sep 15, 2026
EwanC
left a comment
EwanC
left a comment
Contributor
There was a problem hiding this comment.
Minor nit but otherwise LGTM, nice perf improvement.
The wgrad window spans the y gradient's spatial extent, so expanding it held two tap buffers of that size per worker thread. ConvolutionWindow now keeps only the valid taps of each dimension: fprop and dgrad expand() the kernel-sized product, and wgrad walks it with forEachTap(), bounding its scratch by the sum of the extents. Both produce taps in the same row-major order, so accumulation order and results are unchanged. - Move the per-thread work loop and the bool early-exit probe into ParallelTensorRange::forEachIndex, shared by both parallel functors. - Add a nullable validateNoRaggedTensor overload for optional arguments. - Convolution: use flatOffset for output offsets, size_t spatial rank, role names (dx, dw, dy) in ragged errors, and a class-scope PREFIX. - Test the reused index buffer, forEachTap against expand(), and make the scratch isolation test hold every worker at a barrier so thread stack reuse cannot alias two scratches. - Replace comments that described the previous implementation with the contracts they rely on.
…idation The CPU convolution kernels address x, w and y through hoisted base pointers, so a y whose batch differs from x makes wgrad read past the end of x, and a y whose channel count differs from w's output channels makes fprop write past the end of y. Reject both in validateConvolutionParams, which the GPU reference shares.
…references The references address every tensor through hoisted base pointers and flat offsets computed from x's shape, so a tensor of the right rank but the wrong shape is read or written out of bounds. The earlier rank guards did not catch that; for example layernorm fprop indexed rstd with mean's batch walk without comparing the two. Validate shapes instead of ranks: - Layernorm: y (or x and dx) must match the input; scale, bias, dscale and dbias share one shape, the normalized dims optionally preceded by 1s; mean and rstd share one shape, the batch dims optionally followed by 1s, with at least one dimension. - RMSNorm: y, dy and dx must match x; bias, dscale and dbias must match scale; invRms must be x with the normalized dims set to 1. Validation is now stricter than TensorBase::getIndex's argument-count check: an output of higher rank than the input is rejected, deliberately. With shapes validated, layernorm walks the normalized and batch dims of x directly, which removes the zero-padded walk-stride vectors. bprop keeps the per-batch statistics in one row-major table filled by pass 1, whether read from mean/rstd or recomputed, so pass 2 no longer branches per element and the batchStrides resize and tmpBatchOffsets table are gone. The Welford loop shared by fprop and bprop is one helper. All error messages carry the reference's PREFIX. The rank-validation tests move with the ragged-rejection tests into TestCpuFpReferenceInputValidation.cpp, with one test per shape guard.
adickin-amd
commented
Sep 25, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
Summary
The CPU float references for convolution, layernorm and RMSNorm built a fresh
std::vector<int64_t>for every element — and, in convolution, for every MAC — then resolved it throughgetHostValue/setHostValue, which costs a virtualmemory()call plus a stride reduction on top of the allocation. Runtime tracked allocation count rather than arithmetic, and the Windows heap serialised what glibc's per-thread cache absorbed, leaving convolution 22-35x slower on Windows than Linux. This replaces the per-element index vectors with flat offset tables built once per call, which makes the references 10-270x faster with bit-identical results, and removes one of the reasons the shapes in PR #11884 were parked in heavier tiers.Tracking reference to be added before this leaves draft.
Risk Assessment
Low risk. Test-only code: these headers are consumed by
test_sdkand the provider integration/gpu-ref suites, never by shipping library code, so no public API, ABI, schema, build configuration or dispatch behavior changes. The references are the oracle for provider correctness tests, so a defect here would surface as widespread test failures rather than silent product breakage — and the change was verified bit-identical to the previous implementation rather than merely within tolerance.Two deliberate behavior changes, both making previously undefined behavior loud: ragged tensors are now rejected (flat stride arithmetic bypasses
getIndexImpl, whichRaggedTensorBaseoverrides to rebase each batch atragged_offset[b]), and inputs whose shapes do not match are now rejected up front (see Technical Changes). The shape checks are deliberately stricter than the argument-count checkTensorBase::getIndexused to run: an output of higher rank than its input, whichgetIndexaccepted, is now rejected. No caller in the repository passes one.ASIC Coverage
Passing PR CI is sufficient; no dedicated multi-arch sweep is required. The diff is host-side C++ in header-only test utilities. It contains no device code, no kernel selection, no support-surface change and no arch-scoped branch, and it does not alter which cases run or what they assert — outputs are bit-identical. The provider integration lanes that consume these references exercise them on every ASIC as a side effect of normal PR CI.
Testing Summary
mean/rstdare absent.test_sdkunit tests, covering the references directly and through the CPU graph executor plans, plus new suites for ragged rejection, shape validation (one test per guard), and the new parallel-functor, convolution-window and offset-table helpers.hipdnn_gpu_ref_tests, validating each CPU reference against its GPU counterpart across the shape catalogs and fp32/fp16/bf16 instantiations.Testing Checklist
cmake --build build --target hipdnn_test_sdk_teststhen run the binary - ASICs: gfx1151 - Status: Passed (2153 passed, 3 skipped)hipdnn_gpu_ref_tests --gtest_filter=Quick/*:Standard/*- ASICs: gfx1151 - Status: Passed (4292 passed; up from 3846 after merging develop, which added GPU references)hipdnn_gpu_ref_tests --gtest_filter=TestConvolutionValidation.*- Status: Passed (14 passed)Full-hipdnn_gpu_ref_tests --gtest_filter=Standard/*Conv*:Full/*Conv*- ASICs: gfx1151 - Status: Passed at c53c768 (756 passed); not rerun after the review follow-upshipdnn-tidy- Status: Clean for this change (one pre-existing finding inPlatformUtils.windows.hpp:239, from develop)Technical Changes
fprop/dgrad/wgradhoist raw base pointers and strides and index flat offsets, following the pattern already inCpuFpReferenceMoeGroupedMatmul.hpp.detail::ConvolutionWindow. Every spatial dimension maps its window index to a source index independently, and independently decides the tap has no source element — it lands in padding, or fordgradis not stride-aligned — sobuild()resolves each dimension's valid taps once per output element rather than per (output element x channel x tap). fprop and dgradexpand()the kernel-sized product into a tap list reused across channels. wgrad's window spans the output's spatial extent, so it walks the product withforEachTap()instead, keeping per-thread scratch proportional to the sum of the extents rather than their product. Both produce taps in the same row-major order.detail::buildDenseOffsetsanddetail::flatOffsetfor the normalization references, which hold one index space fixed and walk the other. Each tensor is walked with the slice of its own strides that the walk covers, which the shape validation below guarantees lines up with the input.ParallelTensorFunctorWithScratch<Scratch, F>beside the existing factory, giving each worker thread its own scratch instance for the whole of its work range. The plain functor invokes its callee with indices only, so a callee needing state that outlives one work item previously had to reach for athread_local. The work split and the per-thread loop live once inParallelTensorRange::forEachIndex, which both functors use.bool-returning functor takingstd::vector<int64_t>&no longer has its result silently discarded.iterateAlongDimensionsproduced, so floating-point accumulation order is unchanged and results are bit-identical.TensorBase::getIndexalso skipped its argument-count guard and, in debug builds, its bounds check, so a same-rank but mis-shaped tensor was read or written out of bounds. Each reference now validates shapes, not just ranks. A mismatch is a caller error and throws; nothing is clamped.y(fprop) andx/dx(bprop) must match the input; scale, bias, dscale and dbias share one shape, the normalized dims optionally preceded by 1s; mean and rstd share one shape, the batch dims optionally followed by 1s, with at least one dimension (a rank-0 tensor holds no element).y/dy/dxmust matchx; bias, dscale and dbias must match scale; invRms must bexwith the normalized dims set to 1.validateConvolutionParams, shared with the GPU reference, also rejects aywhose batch differs fromxor whose channel count differs fromw's output channels.detail::validateNoRaggedTensor, as the MoE references already did. The graph executor rejects them one level up; this covers direct callers such as the samples and the gpu-ref fixtures.bpropon a whole-tensor normalization withoutmean/rstd: the previous implementation reduced a padded batch index against an empty stride vector and segfaulted. bprop now keeps the per-batch statistics in one row-major table filled by pass 1, whether read frommean/rstdor recomputed, so pass 2 no longer branches per element. The Welford loop shared by fprop and bprop is one helper.buildFullIndicesand the parallel functor's unusedgetNdIndices.Measured effect
Calling the references directly on Windows / gfx1151, fp32, measured at c53c768 (before wgrad switched to
forEachTap()):ResNeXt2GroupResNeXt2GroupResNeXt2GroupStride2Med3d(3D)32x32x32x28x6232x32x32x28x6216x64x225x22516x64x225x225At suite level,
Standard/*Conv*inhipdnn_gpu_ref_testsdrops from 40.50 s to 8.36 s, and the 816 Quick+Standard layernorm and RMSNorm cases from 182.8 s to 145.3 s. The norm suites are now bounded by GPU-side reference execution and tensor setup rather than by the CPU reference.Note on scope: the sped-up shapes are not yet reachable by any CTest category —
dnn-providers/integration-tests/test_categories.yamlstill hascomprehensiveandfullcommented out pending a separate ROCm-side change. This removes one contributing reason those shapes were parked; it does not unpark them.