-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[GPU] Refactor onednn::layout_to_memory_desc function #33100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[GPU] Refactor onednn::layout_to_memory_desc function #33100
Conversation
fddcebe to
e6fa4ba
Compare
e6fa4ba to
52ff74f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the onednn::layout_to_memory_desc function by decomposing it into specialized variants based on specific usage patterns. The original function used bit flags (mem_flags) to control different behaviors, which has been replaced with dedicated functions for each use case.
Key Changes
- Replaced flag-based
layout_to_memory_descfunction with specialized variants:layout_to_memory_desc_flatten,layout_to_memory_desc_blocked,layout_to_memory_desc_grouped, andlayout_to_memory_desc_strides - Refactored dimension and stride calculation logic into separate helper functions with switch statements instead of if-else chains
- Updated all call sites across multiple files to use the appropriate specialized function instead of passing flags
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils.hpp | Added declarations for new specialized layout_to_memory_desc variants and updated signature of flag-based function |
| utils.cpp | Implemented new specialized functions with refactored helper functions for dimension and stride calculations; commented out original implementation |
| program_node.cpp | Updated binary post-op creation to use ternary operator with specialized functions instead of flag-based calls |
| reorder_onednn.cpp | Replaced flag-based calls with layout_to_memory_desc_blocked |
| reduce_onednn.cpp | Replaced flag-based calls with layout_to_memory_desc_blocked |
| primitive_onednn_base.h | Updated fused operation handling to use ternary operator with layout_to_memory_desc_flatten |
| fully_connected_onednn.cpp | Replaced flag-based calls with layout_to_memory_desc_flatten and conditional logic for weights handling |
| deconvolution_onednn.cpp | Replaced flag-based calls with layout_to_memory_desc_flatten for bias |
| convolution_onednn.cpp | Replaced flag-based calls with layout_to_memory_desc_flatten and added explicit target_fmt parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dilation.insert(dilation.end(), insert_count, 0); | ||
| pad_l.insert(pad_l.end(), insert_count, 0); | ||
| pad_r.insert(pad_r.end(), insert_count, 0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random spot)
Some thoughts about this PR:
- Separation of
layout_to_memory_desclooks good. Especially againstuse_stridescase. - Does it cleanup the lambda function that was introduced in https://github.com/openvinotoolkit/openvino/pull/32391/files?
layout_to_memory_descis separated into multiple functions, but it is implemented based oncalculate_memory_dims. Does it bring value?- I think some call path is too deep. Is it inevitable? For example:
layout_to_memory_desc_grouped -> calculate_memory_dims -> calculate_default_dims -> calculate_3d_tensor_dims
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
e312718 to
2c7fdd6
Compare
+ Enhance readability and maintainability
+ Add unit tests
2c7fdd6 to
94e8c02
Compare
Description of the issue
onednn::layout_to_memory_desc()function to improve readability and maintainability.The code and line that caused this issue
openvino/src/plugins/intel_gpu/src/graph/impls/onednn/utils.hpp
Line 48 in 6b0c6a3
Checklist
Tickets: