-
Notifications
You must be signed in to change notification settings - Fork 8
09132024 upstream fwd bwd optimized #74
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: 09132024_upstream_main
Are you sure you want to change the base?
09132024 upstream fwd bwd optimized #74
Conversation
…u/hip_kernel_inc/
This reverts commit cb8cfc9. This is not due to missing files but broken #include paths
* Rename common header * Delete redundant files related to fwd pass * Add hip bwd kernel template generation * Remove old instantiation code, clean-up leftovers (except debug prints)
Summary: X-link: https://github.com/facebookresearch/FBGEMM/pull/244 - Fix logging in benchmarks (redo D62973297 bc diff cannot be unlinked) Reviewed By: basilwong, spcyppt Differential Revision: D62983422 fbshipit-source-id: 9551cf15df7c1c0281193758b25aaae177e34b67
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.
Need more clean up imo.
| kEmbeddingDim, | ||
| kWeighDecayMode | ||
| ) | ||
| }} |
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.
I like this. No indents look weird at first glance but much more readable than nested inner loop.
|
|
||
| {%- if weighted %} | ||
| vals[_j*kMaxVecsPerThread + i] = weights_slice; | ||
| //{%- if weighted %} |
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.
Consider removing them if they are not useful anymore.
If it's some upcoming feature, we can add them back later.
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.
Done
| {%- endif %} | ||
| {%- endmacro %} | ||
|
|
||
| {#-/* |
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.
There is one question. Should we keep the comment in generated source files?
(Just a concern, not a blocking factor)
fbgemm_gpu/codegen/training/forward/embedding_forward_split_kernel_template.cu
Outdated
Show resolved
Hide resolved
Definitely. I did a bit, but still more to come |
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.
LGTM
Cleaned-up branch history for easier review. This branch contains merged bwd and fwd pass optimizations and targeting relatively up-to-date upstream. Current limitations:
BWD:
FWD: