backward performance optimization for MI350#4925
Conversation
✅ Deploy Preview for pytorch-fbgemm-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hi @q10 , sorry I missed your message. BTW, we discovered a numerical issue in 986cceb and reverted it in 85417b4. It unblocks merging the bwd optimization first. Thank you. |
I think this commit only addresses the build step, where we need to link to tbb. However, for runtime, you might need to do a find in $CONDA_PREFIX from inside the container, and manually update LD_LIBRARY_PATH, or create a symlink (something like FBGEMM/.github/scripts/utils_build.bash Line 383 in 0d49628 |
Hi @q10 , I can see a few line below of your example that links tbb FBGEMM/.github/scripts/utils_build.bash Line 388 in 0d49628 One more thing is that installing tbb may not be sufficient. |
ionuthristodorescu
left a comment
There was a problem hiding this comment.
I will also send some diffs on Slack.
|
|
||
| // Compute shared memory size for cta_per_row | ||
| constexpr auto kCacheAccBytes = sizeof(at::acc_type<cache_t, true>); | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; |
There was a problem hiding this comment.
Is this line and the one below common between CUDA and ROCm ? If yes, we should the {% if rocm %} guards around them.
There was a problem hiding this comment.
{% if rocm %} guards are already applied. Do we need additional changes on here?
There was a problem hiding this comment.
Let me take a look and review in the final version so that we're on the same page.
| // Compute shared memory size for cta_per_row | ||
| constexpr auto kCacheAccBytes = sizeof(at::acc_type<cache_t, true>); | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; | ||
| int32_t total_L = indices.numel(); |
There was a problem hiding this comment.
See above comment - total_L seems used only in the ROCm path, so move it under ifdef USE_ROCM ?
There was a problem hiding this comment.
Moved int32_t total_L = indices.numel(); doen under {% if is_rocm %}
There was a problem hiding this comment.
I couldn't find the fixes, not sure if it's another PR I missed or it hasn't been pushed. Would you mind sharing the latest version with all the fixes?
There was a problem hiding this comment.
The fix regarding this is already on line 1065 on this PR. This display is outdated.
|
|
||
| const bool use_deterministic_algorithms = at::globalContext().deterministicAlgorithms(); | ||
| const int max_segment_length_per_cta = use_deterministic_algorithms ? INT_MAX : 1024; | ||
| const int max_segment_length_per_cta = use_deterministic_algorithms ? INT_MAX : 4096; |
There was a problem hiding this comment.
This seems to affect the CUDA regular path as well - please use a {% if rocm %} guard to select between 1024 and 4096.
| constexpr auto kCacheAccBytes = sizeof(at::acc_type<cache_t, true>); | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; | ||
| int32_t total_L = indices.numel(); | ||
| #ifdef USE_ROCM |
There was a problem hiding this comment.
USE_ROCM is a ROCm-specific, could we guard it with a {% if rocm %} so it does not bleed into CUDA codegen ?
| FBGEMM_LAUNCH_KERNEL( | ||
| backward_cta_per_row_kernel, | ||
| cta_per_row_grid_size, | ||
| // (64, 2) |
| TORCH_CHECK_EQ(grad_outputs.size(), 1); | ||
|
|
||
| constexpr int32_t max_segment_length_per_warp = 32; | ||
| constexpr int32_t max_segment_length_per_warp = 16384; |
There was a problem hiding this comment.
This path seems common with regular CUDA, could we guard it with a {% if rocm %} guard to select between 32 and 16384?
There was a problem hiding this comment.
The max_segment_length_per_warp passed by host will be modified on embedding_split_host_pt2_autograd_template.cpp later. Reverting max_segment_length_per_warp on here back to 32.
| const auto permute_output_dim_0_1 = | ||
| ctx->saved_data["permute_output_dim_0_1"].toBool(); | ||
|
|
||
| constexpr int32_t max_segment_length_per_warp = 32; |
There was a problem hiding this comment.
This path seems common with regular CUDA, could we guard it with a {% if rocm %} guard to select between 32 and 16384?
There was a problem hiding this comment.
The max_segment_length_per_warp passed by host will be modified on embedding_split_host_pt2_autograd_template.cpp later. Reverting max_segment_length_per_warp on here back to 32.
| TORCH_CHECK(aux_tensor[IDX_LXU_CACHE_LOCATIONS].has_value(), "lxu_cache_locations should have value."); | ||
| const auto lxu_cache_locations = aux_tensor[IDX_LXU_CACHE_LOCATIONS].value(); | ||
| const auto is_experimental = aux_bool[IDX_IS_EXPERIMENTAL_TBE]; | ||
| const auto mixed_D = aux_bool[IDX_MIXED_D]; |
There was a problem hiding this comment.
This path seems common with regular CUDA, could we guard it with a {% if rocm %} guard to select between 32 and 16384?
There was a problem hiding this comment.
I don't think rocm needs to be guarded here. mixed_D has been passed to all. It was not used in the forward function, so it's just being saved for backward.
Please use static_cast<bool>(aux_bool[IDX_MIXED_D]);
then you can replace this line to ctx->saved_data["mixed_D"] = mixed_D
There was a problem hiding this comment.
Applied the suggested code onto latest PR
| // Workaround. Should not be upstreamed in any way. | ||
| // Redistribute all cta_per_row work to warp_per_row. | ||
| int32_t total_L = indices.numel(); | ||
| {%- if (not nobag) and |
There was a problem hiding this comment.
Could we add a {% if rocm %} guard around this code, total_L is used only in the USE_ROCM path and USE_ROCM is ROCm specific (so if we do not guard it with {% if rocm %} it will be codegen'ed for regular CUDA paths as well).
There was a problem hiding this comment.
By adding a jinja, do you mean only applying {% if rocm %} around the total_L, i.e. @ionuthristodorescu
{% if rocm %}
int32_t total_L = indices.numel();
{%- endif %}?
Or changing from using USE_ROCM to {% if rocm %} entirely?
There was a problem hiding this comment.
The source code will generate but it shouldn't be compiled. Are we trying to double check here or does it cause any issues, because CUDA path should not compile this. Besides, I think rocm is not passed in this file as a global variable, so the condition will always be false, and total_L will never show up in the source code.
There was a problem hiding this comment.
I agree with you. Either USE_ROCM or jinja {% if is_rocm %} is fine for us. Please let us know which one we stick with.
There was a problem hiding this comment.
On this file, we should stick with USE_ROCM. I think your original change is correct. rocm is not defined in this file, so jinja will see it as false, and int32_t total_L = indices.numel(); may not show up in the generated source code. Let me review in the final version.
| ) | ||
| {%- endif %} | ||
|
|
||
| for (auto j = 0; j < kWarpSize && l_start + j < L; ++j) { |
There was a problem hiding this comment.
Here we should use a {% if rocm %} guard to select between rolled / unrolled versions of the loop on regular, non-ROCm paths.
| TORCH_CHECK(aux_tensor[IDX_LXU_CACHE_LOCATIONS].has_value(), "lxu_cache_locations should have value."); | ||
| const auto lxu_cache_locations = aux_tensor[IDX_LXU_CACHE_LOCATIONS].value(); | ||
| const auto is_experimental = aux_bool[IDX_IS_EXPERIMENTAL_TBE]; | ||
| const auto mixed_D = aux_bool[IDX_MIXED_D]; |
There was a problem hiding this comment.
I don't think rocm needs to be guarded here. mixed_D has been passed to all. It was not used in the forward function, so it's just being saved for backward.
Please use static_cast<bool>(aux_bool[IDX_MIXED_D]);
then you can replace this line to ctx->saved_data["mixed_D"] = mixed_D
| // Workaround. Should not be upstreamed in any way. | ||
| // Redistribute all cta_per_row work to warp_per_row. | ||
| int32_t total_L = indices.numel(); | ||
| {%- if (not nobag) and |
There was a problem hiding this comment.
The source code will generate but it shouldn't be compiled. Are we trying to double check here or does it cause any issues, because CUDA path should not compile this. Besides, I think rocm is not passed in this file as a global variable, so the condition will always be false, and total_L will never show up in the source code.
| self.pooling_mode != PoolingMode.NONE | ||
| ), "Mixed dimension tables only supported for pooling tables." | ||
|
|
||
| self.mixed_D = mixed_D |
There was a problem hiding this comment.
On this change, I assume mixed_D needs to be accessible as the module parameter? Would it cause any issues if it's not self.mixed_D? Asking this to check if we need to split the PRs for backend (C++ source code and backend codegen) and frontend changes (split_table_batched_embeddings_ops_training.py).
There was a problem hiding this comment.
Optimization on warp_per_row and cta_per_row kernels will not be activated if self.mixed_D is not present.
There was a problem hiding this comment.
mixed_D is set to be true by default through the pipeline. In this case, we should change mixed_D to be False and let the frontend pass the correct mixed_D.
|
|
||
| // Compute shared memory size for cta_per_row | ||
| constexpr auto kCacheAccBytes = sizeof(at::acc_type<cache_t, true>); | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; |
| // Compute shared memory size for cta_per_row | ||
| constexpr auto kCacheAccBytes = sizeof(at::acc_type<cache_t, true>); | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; | ||
| int32_t total_L = indices.numel(); |
There was a problem hiding this comment.
I couldn't find the fixes, not sure if it's another PR I missed or it hasn't been pushed. Would you mind sharing the latest version with all the fixes?
| "_nobag" if nobag else "", | ||
| ) | ||
| for nobag in [ | ||
| True, |
There was a problem hiding this comment.
no nobag for gen_embedding_backward_split_{}{}_device_kernel_hip.hip?
There was a problem hiding this comment.
Not now. The nobag support is planned in near future
| } | ||
| {%- endif %} | ||
| for (; j < kWarpSize && l_start + j < L; ++j) { | ||
| {%- else %} // if is_rocm |
There was a problem hiding this comment.
nit: change to {#-/* if is_rocm*/#}, otherwise it would be misleading in the generated code
| } | ||
| } | ||
| } | ||
| {%- endif %} |
There was a problem hiding this comment.
nit: add {#-/* if not ssd and not dense and not use_vec_blocking and not vbe */#}
| } | ||
| } | ||
| {%- endif %} | ||
| for (; j < kWarpSize && l_start + j < L; ++j) { |
There was a problem hiding this comment.
line 337 and 339, does it really make a difference?
Otherwise, line 337-340 could just be {%- endif %}{#-/* if is_rocm */#}, since line 339 (i.e., for (auto j = 0; j < kWarpSize && l_start + j < L; ++j) {` will be common for both rocm and cuda.
There was a problem hiding this comment.
Done in bf143c7. Now it's unified, so CUDA uses it as a main loop while ROCm uses this loop to handle the tailing iterations
| {%- endif %} | ||
|
|
||
| {%- if is_rocm and not is_index_select and optimizer == "rowwise_adagrad" and not dense and not is_gwd_kernel and not vbe and not ssd %} | ||
| {%- if is_optimized_hip_kernel_supported_mode %} |
There was a problem hiding this comment.
Not now. The nobag support is planned in near future
| } | ||
| {%- else %} | ||
| int32_t num_cta_per_row_groups = kMaxThreads / kWarpSize; | ||
| int32_t work_group_size = kMaxThreads; |
There was a problem hiding this comment.
make work_group_size const for cuda
const int32_t work_group_size = kMaxThreads;
| self.pooling_mode != PoolingMode.NONE | ||
| ), "Mixed dimension tables only supported for pooling tables." | ||
|
|
||
| self.mixed_D = mixed_D |
There was a problem hiding this comment.
mixed_D is set to be true by default through the pipeline. In this case, we should change mixed_D to be False and let the frontend pass the correct mixed_D.
In regards to the TBB issue, I think this might be fixed with PR 5087. Could you rebase on latest main after this PR 5087 lands? |
warp per row wg change
Rebase is done |
@liligwu @avbokovoy @q10 We still see the tbb errors on CPU CI? https://github.com/pytorch/FBGEMM/actions/runs/19134142334/job/54730556036 |
We had missed one case, and this should be addressed with PR 5106. This PR should be rebased on latest main once PR 5106 lands |
| const auto lxu_cache_locations = aux_tensor[IDX_LXU_CACHE_LOCATIONS].value(); | ||
| const auto is_experimental = aux_bool[IDX_IS_EXPERIMENTAL_TBE]; | ||
| const auto mixed_D = static_cast<bool>(aux_bool[IDX_MIXED_D]); | ||
| {%- endif %} |
There was a problem hiding this comment.
actually, this is not needed in forward since you use mixed_D in backward. I already fixed this in the internal diff.
| {%- if not nobag %} | ||
| ctx->saved_data["max_D"] = max_D; | ||
| ctx->saved_data["mixed_D"] = static_cast<bool>(aux_bool[IDX_MIXED_D]); | ||
| ctx->saved_data["mixed_D"] = mixed_D; |
There was a problem hiding this comment.
so this should be ctx->saved_data["mixed_D"] = static_cast<bool>(aux_bool[IDX_MIXED_D]);
I already fixed this in the internal diff.
| constexpr int32_t BT_block_size = 64; | ||
| constexpr int32_t max_segment_length_per_warp = 64; | ||
| int32_t max_segment_length_per_warp = 64; | ||
| int32_t total_L = indices.numel(); |
There was a problem hiding this comment.
this should be moved under line 1072, since is only used in line 1076 which only exists under {%- if (not nobag) and (optimizer == "rowwise_adagrad") and (not vbe) and (not is_gwd) and (not ssd) and (not is_index_select) and (not dense) %}
I already fixed it internal diff.
| #include <cassert> | ||
|
|
||
| #ifdef USE_ROCM | ||
| #include "fbgemm_gpu/rocm/split_embeddings_common.h" |
There was a problem hiding this comment.
@liligwu @avbokovoy
This causes problem with internal downstream builds which the code already has const auto BLOCK_SIZE = 64; defined. Adding split_embeddings_common.h which also #define BLOCK_SIZE, caused compilation failure.
Summary: bwd performance optimization for ROCm. Fix numerical issues Reviewed By: leitian Differential Revision: D83116315 Pulled By: spcyppt
Summary: Pull Request resolved: pytorch#5121 bwd performance optimization for ROCm. Fix numerical issues Pull Request resolved: pytorch#4925 Reviewed By: leitian Differential Revision: D83116315 Pulled By: spcyppt
|
Hmm, it looks like we are still seeing tbb linking issues - https://github.com/pytorch/FBGEMM/actions/runs/19376974534/job/56094185131?pr=4925 Im guessing that we need both the tbb installation and the explicit tbb linking. @liligwu sorry for the back and forth, can I ask you to add back the tbb linking in the cmake, and rebase to latest commit on |
bwd performance optimization for ROCm.
Fix numerical issues