Nice work :) #2

Activity

1. pwilkin commented on Jul 28, 2026

   

   pwilkin

   on Jul 28, 2026

   ContributorAuthor

   More actions

   PR is up, also updated numbers - got it all the way up to 68.5.
2. Kaden-Schutt commented on Aug 1, 2026

   

   Kaden-Schutt

   on Aug 1, 2026

   Collaborator

   More actions

   Thank you, and sorry for the slow reply — this sat a week and your PR sat four days, which is not the turnaround a contributor doing this quality of work deserves.

   62 → 68.5 t/s against Vulkan's 70 is a better result than I expected from the graph path alone. Closing roughly 80% of that gap is the number I'd been hoping to see from someone who wasn't me, on a model and harness I didn't pick.

   Your PR is merged (#3), with a small follow-up in 1e87b1a: I re-attached the # Safety doc on replay_and_wait, documented the load-bearing ordering of the native-hipSuccess gate, and handled a discarded Result from update_kernargs so it falls back to force_native instead of silently dropping. I deliberately left your fill(0)/min(len) truncation semantics alone — they're byte-identical to lower_plan_to_pm4_ib, and divergence between the lower path and the update path would be the real defect. I also independently reproduced your claim 1 against stock HIP on gfx1201: hipGraphExecUpdate(ex, g, NULL, &res) returns 1, and (..., &slot, &res) returns 0. You were right.

   Something you should know about what you were running against

   While auditing the interposer I found that hipGraphCreate was handing the application a pointer to our own heap object, not a native ihipGraph*, with the real graph stored inside it. The interposer exported 13 of the 93 hipGraph* entry points ROCm 7.14 provides. A call to any of the other 72 reached real HIP carrying our pointer, which HIP then dereferenced as its own type.

   That matters to you specifically, because ggml/src/ggml-cuda/vendors/hip.h remaps cudaGraphGetNodes, cudaGraphNodeGetType, cudaGraphKernelNodeGetParams and cudaGraphKernelNodeSetParams onto four of those 72. If ggml's CUDA-graph path was live in your runs, those calls were in the hazard. You were getting correct output, so it evidently didn't bite — but "didn't bite" is not "was safe", and you should know it was there while you were measuring.

   It's fixed, in two steps:

   • hipgraph: shim the 72 unexported entry points that took our handles #5 shimmed all 72 to translate handles properly. Three independent reviewers went at it over two rounds and failed 36 of 72 on the first pass, almost all one mistake — the "is this ours" check applied per call instead of per argument.
   • hipgraph: give the application native handles instead of our own pointer #7 removed the root cause instead: the application now holds real native HIP handles, and our state lives in a side table keyed by the native pointer. Type confusion is now structurally impossible, because you never hold one of our pointers. An entry point we haven't thought of degrades to lost acceleration instead of corruption — which is the failure mode an LD_PRELOAD interposer should have.

   Consequence for your integration: three of those four symbols now need no shim at all. HIP answers them directly and correctly. Only KernelNodeSetParams is still intercepted, and only to mark our lowered PM4 plan stale.

   No performance cost

   Verified across the RDNA lineup, A/B against the pre-redesign build, median of 5 runs, correctness checked every run:

   arch	gen	explicit	capture
   gfx1010	RDNA1	1.20× → 1.21×	1.21× → 1.22×
   gfx1030	RDNA2	1.36× → 1.36×	1.37× → 1.37×
   gfx1100	RDNA3	2.02× → 2.02×	2.02× → 2.04×
   gfx1201	RDNA4	1.85× → 2.01×	1.97× → 1.94×

   Worth re-running your llama.cpp numbers on current master when you get a chance — not because I expect a change, but because your harness is a far better test than a microbenchmark, and it's the one measurement I can't make myself.

   There's also CI now (#4), so a PR gets build, test, rustfmt and clippy before anyone looks at it. Yours had to be verified by hand.

   Leaving this open until you've had a chance to re-measure — close it whenever you like.