-
Notifications
You must be signed in to change notification settings - Fork 29
Enable CI lint gh action on ROCm #547
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: dev
Are you sure you want to change the base?
Changes from 1 commit
cfd75e8
4ea0d55
a87ccf4
6748cd8
b60c7f9
caceac3
6bc015c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| Fix cpplint violations in common and PyTorch extension code | ||
|
|
||
| transformer_engine/common/amd_detail/hip_float8.h | ||
| -Host constructor: multi-statement if/else now uses braces (readability/braces). | ||
|
|
||
| transformer_engine/common/cast/mxfp8/rocm_quantize_mxfp8.cuh | ||
| -Include <cstdint>; typedef for gfx950 vector type uses int16_t instead of | ||
| short (runtime/int). | ||
|
|
||
| transformer_engine/common/ck_fused_attn/src/ck_fused_attn_utils.cpp | ||
| -dladdr: avoid ill-formed function-pointer-to-void* cast via a small union | ||
| (readability/casting / portable POSIX). | ||
| -get_ck_log_stream: else branch restructured with nested if so else/brace | ||
| pairing satisfies cpplint (readability/braces). | ||
|
|
||
| transformer_engine/common/fused_attn_rocm/fused_attn.cpp | ||
| -check_set_window_size: replace std::make_pair<int64_t,int64_t>(...) with | ||
| std::pair<int64_t,int64_t>(...) (build/explicit_make_pair). | ||
| -Replace alternative tokens `or` with || (readability/alt_tokens). | ||
| -log_fused_attn_config: same for sliding-window condition. | ||
|
|
||
| transformer_engine/common/gemm/rocm_gemm.cu | ||
| -ObjCache / NameMapper: mark single-argument constructors explicit | ||
| (runtime/explicit). | ||
| -HIPBLASLT scaling_mode check: split #if/#else branches so each if has its | ||
| own braced body; use static_cast<int> instead of C-style cast | ||
| (readability/braces, readability/casting). | ||
| -Debug logging: (int) casts -> static_cast<int> for hipDataType fields | ||
| (readability/casting). | ||
| -ServiceStreamKey: use std::uint64_t alias instead of unsigned long long | ||
| (runtime/int). | ||
|
|
||
| transformer_engine/common/normalization/common.cpp | ||
| -getNormalizationPlan: after optional CUDNN plan, use if (!plan) { ... } for | ||
| TE plans instead of } else #endif if (readability/braces across preprocessor). | ||
|
|
||
| transformer_engine/common/normalization/layernorm/ln_api.cpp | ||
| -Forward/backward: default norm_backend to Te; optional CUDNN path only under | ||
| #ifndef __HIP_PLATFORM_AMD__; set is_aligned only when backend is Te, so | ||
| preprocessor does not split if/else from its braces (readability/braces). | ||
|
|
||
| transformer_engine/common/normalization/rmsnorm/rmsnorm_api.cpp | ||
| -Same pattern as ln_api for forward (including HIP constexpr | ||
| gamma_in_weight_dtype) and backward cudnn vs Te (readability/braces). | ||
|
|
||
| transformer_engine/common/permutation/permutation.cu | ||
| -MoE unpermute kernel: functional-style float(...) casts replaced with | ||
| static_cast<float>(...) (readability/casting). | ||
|
|
||
| transformer_engine/common/util/logging.h | ||
| -NVTE_CHECK_HIPBLASLT macro: std::to_string((int)status) -> | ||
| std::to_string(static_cast<int>(status)) (readability/casting). | ||
|
|
||
| transformer_engine/pytorch/csrc/extensions/gemm.cpp | ||
| -Comm overlap RS path: HIP p2p vs split_overlap_rs restructured with proper | ||
| #else for non-HIP so } else #endif { does not confuse brace rules | ||
| (readability/braces). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,13 @@ void set_aiter_asm_dir() { | |
| static std::once_flag aiter_asm_dir_once; | ||
| std::call_once(aiter_asm_dir_once, []() { | ||
| Dl_info info; | ||
| dladdr((void*)set_aiter_asm_dir, &info); | ||
| // dladdr expects void*; avoid reinterpret_cast<void*>(fn) (not ISO C++). | ||
| union { | ||
| void (*fn)(); | ||
| void *addr; | ||
| } sym{}; | ||
| sym.fn = set_aiter_asm_dir; | ||
| dladdr(sym.addr, &info); | ||
|
Comment on lines
+79
to
+85
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this is unnecessary. Yes, it quiets the warning, but the warning is irrelevant for us granted our support is POSIX focused to begin with. From the the union trick here provides no additional safety -- it's still undefined behavior technically speaking -- and will break in the same circumstances (non-POSIX risk). All things considered, I'd rather we keep things as-is, and if we really want to deal with the warning, we can make a small utility to use pragmas to suppress the warnings locally around the cast. |
||
| const char* log_ck_config_env = std::getenv("NVTE_LOG_CK_CONFIG"); | ||
| bool log_ck_config = log_ck_config_env && std::string(log_ck_config_env) == "1"; | ||
| // Check if user has set AITER_ASM_DIR, if yes, skip auto setting and log | ||
|
|
@@ -130,9 +136,10 @@ std::ostream* get_ck_log_stream() { | |
| if (!log_dir_str.empty() && log_dir_str != "0") { | ||
| if (log_dir_str == "1") { | ||
| log_stream = &std::cout; | ||
| } | ||
| else if (open_ck_fused_attn_log_file(log_file, "ck_fused_attn", log_dir_str)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is a warning for if-else if? I think it is used a lot in our code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The category was readability/braces. The message was along the lines of: “If an else has a brace on one side, it should have it on both.” So the warning showed up because cpplint’s readability/braces heuristic fired on this if / else if layout, not because else if is forbidden. Nesting as else { if (...) { ... } } makes the structure obvious to the linter and cleared the warning.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move them to single line then but do not create nested ifs |
||
| log_stream = &log_file; | ||
| } else { | ||
| if (open_ck_fused_attn_log_file(log_file, "ck_fused_attn", log_dir_str)) { | ||
| log_stream = &log_file; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,7 +117,7 @@ public: | |
| data[key][stream] = item; | ||
| } | ||
|
|
||
| ObjCache(void (*a_offload)(const Data&)): offload(a_offload) {} | ||
| explicit ObjCache(void (*a_offload)(const Data&)): offload(a_offload) {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want these constructors to be explicit? Are they even used implicitly anywhere in our codebase?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they aren’t used implicitly anywhere; the only constructions are direct ObjCache<T,K>(nullptr) from ObjPool and direct init of service_stream_cache with a lambda. explicit was added only to satisfy cpplint runtime/explicit. If we prefer not to mark these callbacks as explicit, we can drop explicit and suppress that line with NOLINT for cpplint instead. There are no APIs that take an ObjCache by value. So explicit is not required for correctness, only for style / tooling. |
||
|
|
||
| ~ObjCache() | ||
| { | ||
|
|
@@ -461,7 +461,7 @@ template<typename T> | |
| class NameMapper | ||
| { | ||
| public: | ||
| NameMapper(const std::unordered_map<T, std::string_view>& name_map): map(name_map) {} | ||
| explicit NameMapper(const std::unordered_map<T, std::string_view>& name_map): map(name_map) {} | ||
| const std::string_view &getName(const T &val) { | ||
| return map.at(val); | ||
| } | ||
|
|
@@ -769,14 +769,17 @@ protected: | |
| } | ||
|
|
||
| #if HIPBLASLT_VERSION_MAJOR > 0 || HIPBLASLT_VERSION_MINOR >= 15 | ||
| if (cfg.scaling_mode < 0 || cfg.scaling_mode >= (int)HIPBLASLT_MATMUL_MATRIX_SCALE_END) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line length and { at the end of the line are understood but lint does not require duplicate body of If.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the duplicate was to fix the readability/braces warning, I have restructured it to compute a bool in preprocessor-only branches, then use one if body.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there is discrepancy here. .clang-format sets line width 100 and this is what IDE uses. So cpplint should be configured accorfingly |
||
| if (cfg.scaling_mode < 0 || | ||
| cfg.scaling_mode >= static_cast<int>(HIPBLASLT_MATMUL_MATRIX_SCALE_END)) { | ||
| std::cout << "[WARNING] Unsupported scaling mode at " << line << "\n"; | ||
| continue; | ||
| } | ||
| #else | ||
| if (cfg.scaling_mode != 0) | ||
| #endif | ||
| { | ||
| if (cfg.scaling_mode != 0) { | ||
| std::cout << "[WARNING] Unsupported scaling mode at " << line << "\n"; | ||
| continue; | ||
| } | ||
| #endif | ||
|
|
||
| auto fp8_filter = te_fp8_fnuz() | ||
| ? [](const hipDataType& val) | ||
|
|
@@ -966,10 +969,10 @@ void hipblaslt_gemm(const Tensor *inputA, | |
| std::cout << "m=" << m << " k=" << k << " n=" << n | ||
| << " transa=" << (param.transA == HIPBLAS_OP_T ? "T" : "N") | ||
| << " transb=" << (param.transB == HIPBLAS_OP_T ? "T" : "N") | ||
| << " A_type=" << (int)(param.Atype) | ||
| << " B_type=" << (int)(param.Btype) | ||
| << " D_type=" << (int)outputD->data.dtype | ||
| << " bias_type=" << (int)inputBias->data.dtype | ||
| << " A_type=" << static_cast<int>(param.Atype) | ||
| << " B_type=" << static_cast<int>(param.Btype) | ||
| << " D_type=" << static_cast<int>(outputD->data.dtype) | ||
| << " bias_type=" << static_cast<int>(inputBias->data.dtype) | ||
| << " grad=" << grad | ||
| << " bias=" << (inputBias->data.dptr != nullptr) | ||
| << " gelu=" << (outputPreGelu->data.dptr != nullptr) | ||
|
|
@@ -1386,7 +1389,7 @@ void hipblaslt_gemm(const Tensor *inputA, | |
| } | ||
|
|
||
|
|
||
| typedef unsigned long long ServiceStreamKey; | ||
| using ServiceStreamKey = std::uint64_t; | ||
|
|
||
| ServiceStreamKey make_service_stream_key(const int device_id, const int cu_count) { | ||
| return (static_cast<ServiceStreamKey>(device_id) << 32) | static_cast<ServiceStreamKey>(cu_count); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -544,24 +544,26 @@ NormalizationPlanBase* NormalizationPlanRegistry::getNormalizationPlan( | |
| plan = std::make_unique<CudnnNormalizationPlan>(NormType, NormStage, wtype, itype, otype, ctype, | ||
| batch_size, hidden_size, sm_count, | ||
| zero_centered_gamma, mode, training); | ||
| } else | ||
| } | ||
| #endif | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get rid of nested ifs. If splitting else-if does not work, better add dummy 'if (false) {' for ROCm instead of 'if (NOrmBAckend...'
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped the nested if (!plan) / inner if (Forward) structure. and added the following more clear structure:
|
||
| if (NormStage == NVTE_Norm_Stage::Forward) { | ||
| plan = std::make_unique<TeNormalizationPlan<ForwardKernelParams>>( | ||
| NormType, NormStage, wtype, itype, otype, ctype, batch_size, hidden_size, sm_count, | ||
| zero_centered_gamma, is_tuned | ||
| if (!plan) { | ||
| if (NormStage == NVTE_Norm_Stage::Forward) { | ||
| plan = std::make_unique<TeNormalizationPlan<ForwardKernelParams>>( | ||
| NormType, NormStage, wtype, itype, otype, ctype, batch_size, hidden_size, sm_count, | ||
| zero_centered_gamma, is_tuned | ||
| #ifdef __HIP_PLATFORM_AMD__ | ||
| , mode, training | ||
| , mode, training | ||
| #endif | ||
| ); | ||
| } else { | ||
| plan = std::make_unique<TeNormalizationPlan<BackwardKernelParams>>( | ||
| NormType, NormStage, wtype, itype, otype, ctype, batch_size, hidden_size, sm_count, | ||
| zero_centered_gamma, is_tuned | ||
| ); | ||
| } else { | ||
| plan = std::make_unique<TeNormalizationPlan<BackwardKernelParams>>( | ||
| NormType, NormStage, wtype, itype, otype, ctype, batch_size, hidden_size, sm_count, | ||
| zero_centered_gamma, is_tuned | ||
| #ifdef __HIP_PLATFORM_AMD__ | ||
| , mode, training | ||
| , mode, training | ||
| #endif | ||
| ); | ||
| ); | ||
| } | ||
| } | ||
| normalizationPlanMap.insert({key, std::move(plan)}); | ||
| return normalizationPlanMap[key].get(); | ||
|
|
||
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'm assuming this won't be part of the final PR?
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.
Yes, This is for me to keep track of the issues fixed, will remove this.