-
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 4 commits
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 |
|---|---|---|
|
|
@@ -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.
It is upstream code. Why change it?