Repository navigation
hrx: a code object has to depend on the headers its kernel includes - #198
bong-water-water-bong merged 1 commit into
Conversation
|
#198 ready for review at
Sibling of #185, different failure: #185 is the loud one (a changed Landing: fork PR → |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY on the code
08eebccef, one file (ggml/src/ggml-hrx/hip/ggml-hrx-hip.cmake +16/-1). The stated root cause is real, and I
reproduced it with this box's own amdclang++:
$ amdclang++ --cuda-device-only --offload-arch=gfx1151 \
--rocm-device-lib-path=/opt/rocm-therock/…/llvm/amdgcn/bitcode \
-x hip -MD -MF k.d -I. -o k.hsaco k.hip # k.hip includes k.h
clang++: warning: argument unused during compilation: '-MF k.d' [-Wunused-command-line-argument]
$ ls k.d
ls: cannot access 'k.d': No such file or directory <- no depfile, while k.hsaco (9048 B) was produced
So DEPFILE is worth nothing on this path, the code object depended on the .hip alone, and a header-only change did
leave every .hsaco stale with the build still reporting success. That is exactly the failure the comment describes.
The glob covers the headers that actually exist on this path. I checked the add-on kernels rather than the in-tree
ones (the in-tree hip/kernels/*.hip include only <hip/hip_runtime.h> and <stdint.h>, so they would pass either
way): addons/hip-hrx/kernels/dense_decode_kquant.hip includes "hrx_build_metadata.h", "hrx_kernel_ids.h",
"nvfp4_decode.h", "q2_0_decode.h" — all quoted, all beside it in the add-on's own kernels/ directory, which is the
per-source "${_source_dir}/*.h" glob. So the metadata fingerprint is covered, which is the one the comment says matters.
Known limits, both accepted by the comment and neither a gate:
- Coarse: any header recompiles every kernel in that directory. For a directory of ~40 decode kernels that is the
cheap side of the trade, and it is declared. - It globs only the kernel's own directory plus the in-tree
hip/kernels/. A kernel including a header from a parent
directory (e.g."../include/x.h") would still be missed. No add-on does that today; worth remembering if one starts.
Not run by me: the end-to-end "touch a header -> the .hsaco rebuilds" in a real HRX build tree — that needs the
-DONEBIT_HRX=ON ExternalProject build, which is a box-sized job, not a review-sized one. The two halves it rests on
(no depfile produced; headers in scope of the glob) are both verified above.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY on the code (re-verdicted at 1c659cdaa)
git diff 08eebccef 1c659cdaa is empty — same tree as the head I reviewed, so the evidence carries: the depfile
claim reproduced on this box (-MD -MF reported unused, no depfile written, .hsaco still produced) and the glob's
scope checked against a real add-on kernel's includes.
amdclang++ writes no depfile under --cuda-device-only: -MD/-MF are reported "unused during compilation", the file is never created, and CMake's transform prints "Expected depfile does not exist". So every code-object custom command depended on the .hip alone, and a header-only change recompiled nothing while the build still reported success - the stale .hsaco keeping the previous bytes of whatever the header defines, hrx_build_metadata.h's per-build fingerprint included. List the headers beside each kernel (and the backend's own hip/kernels/*.h) as explicit dependencies of that kernel's code objects. Coarse on purpose: a header recompiles the kernels that live beside it.
1c659cd to
5c99a0a
Compare
|
#198 ready for review at The conflict was #185 landing where this patch starts ( Re-measured on the new base, both arms rebuilt from scratch (51 code objects now: 47 add-on + the backend's 4,
PR body updated with the same numbers; it merges on the owner's fork word, nothing else needed from me. |
|
Re-ran the A/B inside a real engine build and updated the body — this closes the "not verified: a full engine build with the patched submodule" note at head Engine worktree
Scope: that submodule is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 5c99a0a93
Merge-only rebase: the PR's own contribution (merge-base with 1bit/hrx-vulkan-patched .. head, +/- lines only) hashes to 63a5793d46 at this head and at the head I verdicted (1c659cdaa, base a5c32fbf8) — same hash, same 15 lines, 1 file. Nothing of the change moved; the base did.
Content unchanged from what I verified there: hip/ggml-hrx-hip.cmake adds each kernel's own directory glob plus the backend's hip/kernels/*.{h,hpp,inc,cuh} to the code objects' DEPENDS, with CONFIGURE_DEPENDS, because amdclang++ --cuda-device-only reports -MD/-MF as unused during compilation and writes no depfile — reproduced on this box (the warning: argument unused during compilation: '-MF k.d' and no k.d while the .hsaco is produced). The glob's scope matches what an add-on kernel actually includes (hrx_build_metadata.h and friends sit beside it).
Root cause
amdclang++writes no depfile in--cuda-device-onlymode. Its driver reports the dependency flags as unused and never creates the file:(
-MMDand-M -MFbehave the same. Without--cuda-device-onlya 23 KB depfile is produced, with the fullhip/header closure.)So the
DEPFILEon every code-object rule is inert — CMake'scmake_transform_depfileprintsExpected depfile does not existand writes an emptyCMakeFiles/d/<hash>.d(measured: all three are 0 bytes in a real engine build of this tree,build/hrx/llama/CMakeFiles/d/), and the object depends on nothing but its own.hip.The consequence is silent: a header-only change recompiles nothing, the build reports success, and every
.hsacokeeps the previous bytes of whatever that header defines. The one that makes it visible ishrx_build_metadata.h, whose whole purpose is a per-build 16-byte fingerprint inside every code object: a stale fingerprint is worse than none, because it names a build that is not the one shipped. The same gap applies to the shared device headers the add-on kernels pull in (kq_dot.h,kqt_geometry.h,q2_0_decode.h,nvfp4_decode.h,zaya_fmt.h,fa_prompt_q8_body.inc).Fix
Add the headers that sit beside a kernel — plus the backend's own
hip/kernels/*.h— to that kernel'sDEPENDS. Coarse on purpose (a header recompiles the kernels in its directory); no new files, no compiler flag dependency, and the dead-MD -MF/DEPFILEpair is left alone so it starts working again if a driver ever honours it.Measured (before/after, same tree, same compiler, same kernels)
Reproducer: the real rule file included unchanged into a small project, with the private add-on's
kernels/copied in (47 kernels + their headers/.incs) and the backend's own 4 kernels → 51 code objects,gfx1151, amdclang++ 24 (rocm-10.3.0a20261031). Arm A = the base branch's rule, arm B = this PR on top of it; both build clean (rc 0, 51/51 objects). Neither arm sets a device-library flag — both get it from #185's block, which the base now carries.Then change only
hrx_build_metadata.h(one byte of the 16-byte fingerprint) and rebuild:ninja ggml-hrx-hip-code-objects.hsaconinjaprintsno work to do; the 47 compile exactly once..hipedit still recompiles in one step in B, as before..textsection of the same kernel hashes identically (fa_decodea0eb2e0c…,hrx_gemv_smallfb3cf8f9…, and the base's newhip_set_rows17185149…); the whole-file difference between the arms is only the build directory embedded in.strtab.Not verified
GGML_HRX_HIP_FLAGS/toolchain plumbing is untouched, but the engine's ownONEBIT_HRX=ONtree was not rebuilt for this.hip/kernels) is still not tracked — that would need a real depfile, which this driver does not produce. Everything the tree and the add-on include today lives in those two directories.Rebased onto the current base (17 commits,
428937ae), and re-measured thereThe conflict was #185 landing in the same spot:
e40a3a242added the--rocm-device-lib-pathblock right afterseparate_arguments(_ggml_hrx_hip_flags …), where this patch starts. Resolved by keeping both — #185's blockfirst, then the header globs — so they cannot disagree: #185 decides the flag, this decides the dependencies. Head
5c99a0a93= one commit on428937ae, one file, +15/−1.Re-verified on that base (same reproducer, both arms rebuilt from scratch, no device-library flag in either arm so the
bitcode path comes from #185's block):
428937ae5c99a0a93.textstill identical between the arms for an add-on kernel, a backend kernel and the base's newhip_set_rows;arm B is still idempotent (
ninja→no work to do).Re-run inside a real engine build (closes the "not verified" note)
The isolated reproducer proves the rule; this proves it where it actually runs. Engine worktree
ONEBIT_HRX=ON(forksubmodule at
d8bd0ff), the nested fork build, the tree toolchain'samdclang++, add-onGGML_HRX_HIP_ADDON_DIRpointed at a copy of the 47-kernel gpu-kernels checkout (so 50 code objects: 47 add-on + that commit's 3 backend
kernels). Both arms configured and built clean, then only one byte of
kernels/hrx_build_metadata.hchanged:ninja ggml-hrx-hip-code-objects.hsaco0x42→0x43ninjahrx_gemv_small.hiptouchedggml-hrx-kernel-corpus(the TU that consumes the regeneratedggml-hrx-hip-code-objects.inc) andllama-clibothbuild and link, rc 0, so the added dependencies do not disturb the embed step or its consumers. Engine-specific detail:
at that submodule commit the backend's own
hip/kernels/has no.hfiles, so in an engine build every dependencycomes from the per-kernel-directory glob — which is where
hrx_build_metadata.hand the other shared device headerslive. Scope: the submodule there is
d8bd0ff, not this PR's base, so the applied text is the same +15/−1 rebased ontothe older file (no #185 block in it) — it verifies the rule and the configure path, not literally "engine main ×
428937ae". Everything was restored afterwards (submodule clean, add-on cache entry empty,ninjano-op) and thegpu-kernels checkout itself was never edited.