Reshape: apply the runtime version guard to the legacy OperationBuilder_v8 too - #467
Merged
Anerudhan merged 1 commit intoAug 3, 2026
Conversation
…er_v8 too The graph-API reshape node already gates CUDNN_ATTR_OPERATION_RESHAPE_MODE on detail::get_backend_version(). The v8 builder has the identical defect and is worse off: its reshape_mode member defaults to ReshapeMode_t::VIEW_ONLY rather than NOT_SET (cudnn_frontend_Operation.h:240), so the existing `if (reshape_mode != NOT_SET)` guard is always true and every legacy reshape sends the attribute to whatever runtime is loaded. Against a pre-9.22 library that returns BAD_PARAM and fails the operation. Same shape of fix and same reasoning about semantics: skipping the attribute on a pre-9.22 runtime reproduces that runtime's only behaviour, which is view-only. An explicit LOGICAL request cannot be honoured there, so it is refused rather than silently downgraded. This does not show up in an SDPA repro because the SDPA nodes use the graph API, which is why it was missed when the graph-API half was fixed.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe reshape operation builder now checks the cuDNN runtime version. It rejects explicit logical reshapes before version 9.22, omits the reshape-mode attribute for legacy view-only reshapes, and preserves configured mode handling on newer runtimes. ChangesReshape mode compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
vedaanta
approved these changes
Aug 1, 2026
Merged
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.
Follow-up to the graph-API fix already on
develop, which gatesCUDNN_ATTR_OPERATION_RESHAPE_MODEondetail::get_backend_version()incudnn_frontend/node/reshape.h.The legacy
OperationBuilder_v8reshape atcudnn_frontend_Operation.h:1808has the identicaldefect and is worse off:
reshape_modedefaults toReshapeMode_t::VIEW_ONLYrather thanNOT_SET(:240), so the existingif (reshape_mode != NOT_SET)guard is always true and everylegacy reshape sends the attribute to whatever runtime is loaded. Against a pre-9.22 library that
returns
CUDNN_STATUS_BAD_PARAMand fails the operation.Same shape of fix, same reasoning about semantics: skipping the attribute on a pre-9.22 runtime
reproduces that runtime's only behaviour, which is view-only. An explicit
ReshapeMode_t::LOGICALrequest cannot be honoured there, so it is refused rather than silently downgraded.
It does not show up in an SDPA reproducer — the SDPA nodes use the graph API — which is why it was
missed when the graph-API half was fixed.
Setting attribute 2202 in isolation reproduces the underlying split:
BAD_PARAMon 9.13.1 / 9.18.0 /9.21.0, success on 9.22.0. The graph-API half was verified end to end by driving an SDPA-backward
graph through
validate → build_operation_graph → create_execution_plans → check_supportwith thefrontend headers held constant and only the runtime library swapped; this half has no SDPA path to
exercise, so it is justified by symmetry with that fix rather than by its own A/B.
Found by an independent review of the graph-API patch.
Summary by CodeRabbit