Repository navigation
feat(hipdnn): Implement a container like ProblemDescription to pack problem parameters - #11922
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
3e8d44f to
27668d1
Compare
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟢 Approval recommended
The refactoring preserves existing calculations and provides focused coverage for the new descriptors.
Pull request overview
Introduces ProblemDescription containers to centralize normalization geometry and batch-normalization heuristic inputs.
Changes:
- Adds problem descriptors for Batchnorm, Layernorm, and RMSnorm.
- Refactors plan compilation and configuration helpers to consume descriptors.
- Updates and extends unit tests for derived geometry and heuristics.
File summaries
| File | Description |
|---|---|
tests/.../RMSnorm/TestRMSnormFwdPlan.cpp |
Tests RMSnorm problem geometry. |
tests/.../Layernorm/TestLayernormFwdPlan.cpp |
Tests Layernorm problem geometry. |
tests/.../Batchnorm/TestBatchnormCommon.cpp |
Updates helper tests for the descriptor API. |
plans/RMSnorm/RMSnormFwdPlan.cpp |
Uses the descriptor for forward geometry. |
plans/RMSnorm/RMSnormCommon.hpp |
Defines the RMSnorm descriptor. |
plans/RMSnorm/RMSnormBwdPlan.cpp |
Uses the descriptor for backward geometry. |
plans/layernorm/LayernormUtilities.hpp |
Declares the Layernorm descriptor. |
plans/layernorm/LayernormUtilities.cpp |
Computes Layernorm geometry centrally. |
plans/layernorm/LayernormPlanBuilder.cpp |
Uses centralized workspace geometry. |
plans/layernorm/LayernormFwdPlan.cpp |
Replaces inline forward geometry logic. |
plans/layernorm/LayernormBwdPlan.cpp |
Replaces inline backward geometry logic. |
plans/batchnorm/BatchnormFwdTrainingPlan.cpp |
Packages forward heuristic inputs. |
plans/batchnorm/BatchnormCommon.hpp |
Defines the descriptor and refactors helpers. |
plans/batchnorm/BatchnormBwdPlan.cpp |
Replaces the previous dimension container. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a24c96e to
a866dfb
Compare
EwanC
left a comment
There was a problem hiding this comment.
Looking good so far, couple of minor comments and highlighting places CI is unhappy
0bc2368 to
7d8adc3
Compare
7d8adc3 to
85bfeec
Compare
85bfeec to
45bbb61
Compare
45bbb61 to
49011c7
Compare
TheRock Submodule Bump ActivityNewest first
|
Motivation
During the implementation of the batchnorm fwd training functionality in the HIP kernel plugin in this PR, we had to bring in a few helper functions such as DefaultConfigSpatialMultiple and DefaultConfigSpatialMultiple , which helps us in finding the right kernel config based on the problem heuristics. They were implemented such that the input arguments are the raw problem parameters. In MIOpen, the same functions use the ProblemDescription struct to pass around this information in a more compact form, which is a cleaner approach.
Closes #11921
Technical Details
This PR implements a similar struct (also named ProblemDescription) in the hip kernel provider and replaces all the parameters that are being passed to the helper functions with this object.
Test Plan
Tested through existing and modified tests.
Test Result
Passed tests on an MI200.
Submission Checklist