feat(hipdnn): implement remaining cuDNN frontend graph nodes - #9738
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #9738 +/- ##
========================================
Coverage 69.18% 69.18%
========================================
Files 2762 2762
Lines 454941 454941
Branches 67110 67110
========================================
+ Hits 314733 314737 +4
+ Misses 117324 117322 -2
+ Partials 22884 22882 -2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
LGTM.
Couple minor comments I think should be addressed before merging.
There was a problem hiding this comment.
A note on provenance before the substance: this review was conducted by Cursor, using the hipDNN overlay of our PR quality skill (hipdnn-pr-quality) on top of the ROCm base skill. I've read the whole thing and I'm putting my name on it, but I won't claim I independently verified every one of the smaller claims (the upstream signature comparisons in particular). The big one I do understand, and I think it's real. So treat the blocking item as mine, and the rest as well-founded suggestions worth a look rather than pronouncements.
Nice work, Mitch. This is a well-built PR and I want to be specific about what I liked, because a lot of it is the stuff people usually skip.
The tests are the best part. The unhappy-path cases (ConvFpropMissingParamsFailsValidation, ConvDgradMissingOutputDimFailsValidation) pair with their happy-path twins to prove the constraint is actually real rather than incidental, which is a nice touch. FailStubPoisonsAnOtherwiseValidGraph locks first-error-wins ordering, so it would catch someone reordering the checks in validate(). And AllAttributeClassesConstructAndName is exactly the right tool for asserting a surface exists: drop an alias or a stub and it fails at compile time, not six months later. I also noticed nearly every shape choice carries a comment naming where it came from ("Mirrors native TestGraph.cpp BatchnormNodeCreation shapes"). That paper trail is worth a lot when someone picks this up later and wonders why these numbers.
Two other things I appreciated. The macro-stamped fail-stub approach means the Tier-2 surface can't drift out of sync one node at a time, and the ALMIOPEN-2041 acceptance criteria line up with this PR almost point for point, which made reviewing it much faster. Enabling the flag in the superbuild lanes was the right call too (more on that below, because I think it's better news than the description says).
I'm requesting changes on one thing, plus a few description corrections.
Blocking
Fail-stubs return nullptr, so the documented "surfaces at validate()" contract doesn't hold for real consumer code. The macro in unsupported_nodes.h ends with return {};, which is a null shared_ptr (or an array of them). But idiomatic cuDNN FE code looks like this:
auto o = graph.rope(x, rope_table, cudnn_frontend::graph::RoPE_attributes{});
o->set_output(true).set_uid(4); // null deref, crashes hereThe consumer segfaults on the next line, before validate() is ever reached. That's the path this PR exists to make graceful, so I think it matters. It also diverges from cuDNN, where node methods always hand back a real tensor (Graph::slice and Graph::transpose in upstream graph_interface.h both assign output_tensor(...) and return it unconditionally, as does Graph::concatenate). And the tests don't catch it because all eight fail-stub tests discard the return value and go straight to validate(), which is the one shape that avoids the crash.
Either fix works for me. Return a placeholder tensor registered on the graph so the ->set_output(...) chain survives and the error surfaces where you documented it; or, if nullptr is deliberate, say so plainly in the header, drop "never silently" from the description, and add a test that pins the null contract. I'd lean toward the first since it makes the promise true and looks cheap.
Important
sdpa_fp8_backward doesn't match upstream, and the MXFP8 overload is missing. Against current upstream cudnn-frontend main (FE 1.26), graph_interface.h declares two overloads: 18 tensor params returning array<...,7> (FP8) and 17 returning array<...,6> (MXFP8). This PR has one stub with 15 params. I checked whether it was systemic and it isn't, which is why I think it's just an oversight: bn_finalize (6 to 6), dbn_weight (5 to 5), genstats (1 to 2), instancenorm, adalayernorm, rope, slice, transpose, concatenate, both moe_grouped_matmul forms, and notably both sdpa_fp8 overloads all match exactly. One caveat: the PR doesn't say which cuDNN or FE version "v9" pins to, and FE arity has grown across 1.x, so this could be version skew. Either way a hipified consumer calling it won't compile, which is the thing we're trying to prevent. Could we pin the targeted FE version somewhere in the shim so this is mechanically checkable?
The CI note in the description is stale, and the reality is better than it says. The description says no PR-CI lane enables the flag and that shim coverage is local-only, deferred to a later ticket. But this PR is what changes that: it adds -DHIPDNN_ENABLE_CUDNN_COMPATIBILITY=ON to both jobs in hipdnn-superbuild-ci.yml, and both run ctest --output-on-failure. I pulled the log from run 30487499647 to confirm it really executed: hipdnn_frontend_tests ran as Test #5 and passed, and hipdnn_sample_cudnn_shim_conv_fprop/_matmul/_layernorm/_pointwise ran as Tests #45 to #48 and passed (52 of 52 green, on gfx1151 Windows and gfx94X Linux). So you can just delete the caveat. Worth fixing because as written it asks a reviewer to grant a coverage waiver you don't need. Separately, the workflow change isn't in the Technical Changes list at all, and it's also what pulled in the extra code-owner review.
"Entirely behind the opt-in flag" isn't quite accurate. LayernormBackwardAttributes.hpp and ResampleFwdAttributes.hpp add Layernorm_backward_attributes and Resample_attributes to the native public headers with no #ifdef guard, and the flag defaults OFF. So those two cuDNN spellings land in hipdnn_frontend::graph for everyone regardless of the flag. I'm fine with the code (there's precedent right above it, Resample_fwd_attributes is already unguarded), but I'd rather we say yes to cuDNN spellings in the native namespace on purpose than have it arrive as a side effect. Just fix the claim.
Minor related note: I'd call this Risk 3 rather than 2. New public header, ~18 node methods, 22 attribute classes, two unguarded native typedefs, and a CI workflow change. Still nowhere near a 4, given it's off by default and host-only.
Suggestions
- Mode flip ordering. The new Tier-1 methods set
_mode = Mode::Native;before forwarding, but the existingsdpa/sdpa_backwardset it after the forward returns. If a hipDNN node method throws, the new ordering leaves the wrapper claiming Native with no node added, so a laterexecute()forwards into an empty graph instead of returningnoExecutionPlanError(). I bet exceptions are a real concern here, since this PR wrapsSdpaRoundTrip.cppmain in try/catch. Moving the assignment after the forward makes it consistent and costs nothing. - Sample consistency. You added the try/catch guard to
SdpaRoundTrip.cpp, but the four new samples in the same directory don't have one, and they deref the node result unchecked (y->set_output(true).set_uid(3);inConvFprop.cpp). Either guard all five or note why SdpaRoundTrip is the special one. UnsupportedAttributes::_computeDataTypeis write-only.set_compute_data_typestores it and nothing reads it, unlike_namewhich hasget_name. If cuDNN's attribute types exposeget_compute_data_type(), hipified source calling it won't compile, which is the same flavor of gap as the blocker. Add the accessor, or drop the field and accept-and-discard.- Ticket AC. ALMIOPEN-2041 says each implemented node lands with a sample mirroring the cuDNN FE sample. Unit-test coverage is complete, but there are 4 samples for ~18 Tier-1 nodes. Can you confirm whether four representative ones satisfy the AC, or the rest are follow-up? Mostly so the ticket can close cleanly.
Future work (not blocking)
Runtime coverage hits 8 of the ~21 stamped fail-stub methods; the rest are compile-checked only. Sampling is fine while they all come from one macro. If any stub ever grows a per-node body, a table-driven test over the stub list would close it cheaply.
Merge logistics
A few non-code things for when the above is settled.
- The eleven failing
codecov/project/*checks are not yours. They're carryforward flags from base commit0e2e6b4for components you don't touch.codecov/patchandcodecov/project/hipDNNare both green. - I'd rebase and re-run before merging. 115 commits have landed on
developsince the merge-base, and two of your files overlap with filesdevelopalso changed:frontend/tests/CMakeLists.txtandsamples/CMakeLists.txt. Both sides are appending to the same source and test-registration lists, which is exactly the combination neither PR's own CI can see. CI here is also about six days old. - Brian's approval predates your last few commits, and the workflow change added another code-owner requirement, so that'll need a second look regardless.
None of this is a lot of work, and the substance is in good shape. Happy to re-review quickly once the fail-stub return question is settled.
1c57865 to
8394cbd
Compare
geomin12
left a comment
geomin12
left a comment
There was a problem hiding this comment.
from a CI perspective, this looks good! however, I'll defer to Tony + Brian for final reviews and approvals
|
Thanks — the blocking item was real and the fix was cheap, so I took your first option. Blocking — fail-stubs returned
|
cd2451f to
b4f84fd
Compare
There was a problem hiding this comment.
Reviewed again by Cursor using the hipDNN overlay of the PR quality skill, with me (Tony) reading and agreeing with the conclusions.
Approving. All of it addressed, and I checked the code rather than taking it on trust.
The blocking fix is right. makeUnsupportedNodeResult mints live tensors through the graph's own tensor(), and I verified the ordering risk that introduces: validate() returns the recorded error before validateOwnedTensors() runs, so the dimless placeholders can't mask GRAPH_NOT_SUPPORTED with a spurious ATTRIBUTE_NOT_SET. The three chaining tests cover exactly the shape the old tests all avoided, single-output and per-element array-output both.
sdpa_fp8_backward now has both overloads (18 to array<...,7>, 17 to array<...,6>), mode flip is after the forward in all of them, all five samples are guarded, and get_compute_data_type() is there with the all-attributes test covering both accessor pairs.
The static_assert on the pin is better than what I asked for. I asked you to reconcile one signature; you made that whole class of drift impossible to land quietly. Thanks also for the version correction (1.24.0, not 1.26) and for running the arity diff mechanically instead of by hand like I did.
Fair correction on _computeDataType too. My guess at why it was write-only was wrong, and yours is the actual reason.
CI backs it up on b4f84fd: both superbuild lanes green with the flag on, TestCudnnShimGraphNodes.cpp compiled into hipdnn_frontend_tests, 56/56 ctest including the four shim samples. Description matches reality now.
One non-blocking ask: drop the follow-up ticket ID in here once you file the remaining-samples issue (I don't see it in Jira yet). I agree with scoping it out rather than growing this PR. I'd just rather the AC gap be tracked than remembered.
|
Alright, I've added the remaining sample work to JIRA ID: ALMIOPEN-2043. In addition to the samples, the story has work to pull down the cudnn-frontend v9 API samples as part of a CI workflow and ensure they compile. The in-tree samples will be an early warning, but the CI step will be the final gate. |
4 participants
Summary
JIRA ID : ALMIOPEN-2041
Feature Flag Protected: Yes - HIPDNN_ENABLE_CUDNN_COMPATIBILITY
Completes the cuDNN-frontend graph node surface on the compatibility shim added in #9361, so
hipified cuDNN v9 consumers can build the full set of
graph::Graph::*node methods againsthipDNN. Tier-1 nodes with a real hipDNN engine forward to the wrapped frontend graph; the
remaining cuDNN v9 nodes are fail-stubs that record
GRAPH_NOT_SUPPORTED(surfaced at the nextvalidate()) while still handing back a live, graph-registered placeholder tensor — so hipifiedsource compiles, survives the idiomatic
node(...)->set_output(true).set_uid(n)chain, and failsloudly at
validate()instead of dereferencing null.The shim targets cuDNN frontend v1.24.0 (
CUDNN_FRONTEND_VERSION == 12400, declared inhipdnn_compatibility/cudnn/cudnn_frontend_version.h).detail/graph_wrapper.hstatic_assertson that macro, so the pin cannot move without a deliberate re-diff of every node signature against
upstream
graph_interface.h/node_interface.h.Risk Assessment
Risk 3 (low-moderate). Header-only and host-only: no kernel selection, dispatch, or shipping
default changes. The cuDNN-shaped surface — new public header, ~18 Tier-1 node methods, 22
fail-stub attribute classes — is behind the opt-in
HIPDNN_ENABLE_CUDNN_COMPATIBILITYflag(default OFF). Two cuDNN spellings land unguarded in the native
hipdnn_frontend::graphnamespace regardless of the flag:
Layernorm_backward_attributesandResample_attributes,typedefs of existing types added alongside the already-unguardedResample_fwd_attributes.That is intentional — cuDNN spellings are welcome in the native namespace — not a side effect of
the shim. Tier-1 node methods are thin forwarders to existing, already-tested hipDNN engines; the
only new runtime behavior is the fail-stub record-and-return-placeholder path, which is
host-covered. Also changes the superbuild CI workflow (enables the flag in both jobs).
ASIC Coverage
ASIC-independent. Frontend source-compatibility plumbing that lowers cuDNN-spelled graphs onto the
existing hipDNN frontend; changes no kernel selection, support surface, or default behavior. All
tests validate host-side with no device; the four samples are host-only graph-build+validate. No
multi-arch sweep required.
The shim, its tests, and its samples build only with
HIPDNN_ENABLE_CUDNN_COMPATIBILITY=ON, andthis PR is what turns that on in CI: it adds
-DHIPDNN_ENABLE_CUDNN_COMPATIBILITY=ONto both jobsin
hipdnn-superbuild-ci.yml, both of which runctest --output-on-failure. Shim coverage istherefore exercised by PR CI on gfx1151/Windows and gfx94X/Linux, not just locally.
Testing Summary
TestCudnnShimGraphNodes.cpp): Tier-1 nodes build a well-shaped graph thatvalidate()s good; Tier-2 fail-stubs recordGRAPH_NOT_SUPPORTEDwith the issue-tracker message;a poison test proves a fail-stub overrides an otherwise-valid graph (first-error-wins); a
compile-time check that all 39 cuDNN v9
*_attributesclasses construct and chain.set_name.reshape) and multi-output stubs(
genstats,sdpa_fp8) hand back live placeholders that survive->set_output(true).set_uid(n)beforevalidate()reportsGRAPH_NOT_SUPPORTED— the shapethat a null return would crash on.
sdpa_fp8_backwardoverload tests: both the 18-tensor FP8 form (7 outputs) and the 17-tensorMXFP8 form (6 outputs) resolve and record, pinning arity and output count against the FE v1.24.0
signatures.
Matmul, Layernorm, Pointwise.
Testing Checklist
HIPDNN_ENABLE_CUDNN_COMPATIBILITY=ON, ranhipdnn-unit-check- Status: Passedctest -R hipdnn_sample_cudnn_shim_- Status: PassedStatus: Passed
Technical Changes
detail/graph_wrapper.h(conv fprop/dgrad/wgrad, batchnorm +backward + inference, layernorm + backward, rmsnorm + backward, matmul, pointwise, reduction,
resample, block-scale quantize/dequantize), each forwarding to the wrapped hipDNN graph and
flipping the graph to Native mode.
detail/node_wrappers/unsupported_nodes.h: a CRTPUnsupportedAttributesbase plus macrosthat stamp Tier-2 fail-stub attribute classes and matching node methods recording
GRAPH_NOT_SUPPORTEDfor the cuDNN v9 nodes with no hipDNN engine yet. Fail-stubs returngraph-registered placeholder tensors (single or array, per the node's upstream return type) so
the recorded error surfaces at
validate()rather than as a null dereference at the call site.detail/graph_wrapper.hstatic_asserts onCUDNN_FRONTEND_VERSION(v1.24.0), making node-signature drift against upstream a build failurerather than a source-compatibility break for consumers.
sdpa_fp8_backwardto upstream's two overloads — 18 tensor params returningarray<...,7>(FP8) and 17 returningarray<...,6>(MXFP8) — matching the existingsdpa_fp8pair.
<shim>::graphincudnn_frontend/graph_properties.h.Layernorm_backward_attributesandResample_attributeson thenative attribute headers (unguarded, as with
Resample_fwd_attributes).HIPDNN_ENABLE_CUDNN_COMPATIBILITY=ONin both jobs of.github/workflows/hipdnn-superbuild-ci.yml, so PR CI builds and runs the shim tests andsamples.
HIPDNN_ENABLE_CUDNN_COMPATIBILITYin the frontend-tests and samples CMake._mode = Mode::Native;flip after the forwarded call in all 18 Tier-1 node methods,matching the pre-existing
sdpa/sdpa_backwardordering: a throwing forward can no longerleave the wrapper claiming Native with no node added.
get_compute_data_type()to the Tier-2UnsupportedAttributesbase, so a stub exposes thesame universal accessor pair as the hipDNN attribute types the Tier-1 aliases resolve to.
try/catchasSdpaRoundTrip.cpp.