[hipdnn] Warn when conv wgrad/dgrad infers output dims assuming groups=1#7263
Merged
BrianHarrisonAMD merged 4 commits intoMay 12, 2026
Merged
Conversation
assistant-librarian
Bot
added
the
external contribution
Contributor
|
Thanks for the contribution @1wos! Heads up, I have updated to latest develop, and enabled the CI run. |
BrianHarrisonAMD
approved these changes
May 11, 2026
Contributor
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
Looking good to me!
Minor nit phrasing that would be nice to get in, but not a huge deal.
Author
|
Thanks for the suggestion! Updated both notes. |
Contributor
|
Kicking off CI run, and will merge once it passes. Thanks for the contribution! |
amontoison
pushed a commit
to amontoison/rocm-libraries
that referenced
this pull request
May 13, 2026
…s=1 (ROCm#7263) Closes ROCm#5259. The two `infer_properties_node()` implementations silently assume `groups = 1` when computing `dw[1]` / `dx[1]`, which gives the wrong channel count for grouped convolutions unless the caller passes `dw` / `dx` shapes explicitly. This PR makes the assumption visible. Each `infer_properties_node()` now emits a `HIPDNN_FE_LOG_WARN` describing what was assumed and how to override it, and the inline comments are rewritten to match. The same caveat is added as a `@note` on `conv_dgrad` and `conv_wgrad` in `Graph.hpp` so the limitation shows up in the public docs. No behavior change for non-grouped convolutions or for callers that pass explicit `dw` / `dx` dimensions. Grouped convolutions that previously relied on inference now log a warning; the inferred channel count itself is unchanged by this PR, since fixing it would require a separate API discussion. --------- Co-authored-by: BrianHarrisonAMD <169072757+BrianHarrisonAMD@users.noreply.github.com>
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
2 participants
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.
Closes #5259.
The two
infer_properties_node()implementations silently assumegroups = 1when computingdw[1]/dx[1], which gives the wrong channel count for grouped convolutions unless the caller passesdw/dxshapes explicitly.This PR makes the assumption visible. Each
infer_properties_node()now emits aHIPDNN_FE_LOG_WARNdescribing what was assumed and how to override it, and the inline comments are rewritten to match. The same caveat is added as a@noteonconv_dgradandconv_wgradinGraph.hppso the limitation shows up in the public docs.No behavior change for non-grouped convolutions or for callers that pass explicit
dw/dxdimensions. Grouped convolutions that previously relied on inference now log a warning; the inferred channel count itself is unchanged by this PR, since fixing it would require a separate API discussion.