-
Notifications
You must be signed in to change notification settings - Fork 224
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
[ROCm 6.1 RC] Fix build failures. [quality] Reorg standard includes in HIP sources. #2637
Changes from all commits
309309b
61e1721
58c4d97
39572c0
4a011d1
58f8d80
7eefad5
ed1a732
8053a0c
f958b46
08a1ca3
699cf08
3e847da
72eb180
10e9581
f498789
8bc23eb
69c0d99
aa09f36
cf02a64
ebeef1d
ee34b45
dab5d3b
c6b4352
b35b497
9990a5d
5b00521
0133956
dc55bba
0d48eb3
19b1a95
592d5cb
e74ed6d
f92cd4c
b81d3a5
ee4020e
f669dca
1ca863f
5756538
c81a237
50740d0
ae88a45
4a3b7ce
d0045b2
1992ab3
d049055
cae396d
f6502c3
d1c0b96
f3e11f0
f9a2cd0
44fd4ba
e59ae19
d349c2d
d445a0b
0d03807
af0eed9
3badc26
0bcbe48
b0df124
afcece4
07e095e
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 |
---|---|---|
|
@@ -380,23 +380,27 @@ if( MIOPEN_BACKEND MATCHES "OpenCL" OR MIOPEN_BACKEND STREQUAL "HIPOC" OR MIOPEN | |
kernels/Conv_Winograd_v30_3_1_gfx11_fp32_f3x2_stride1.inc | ||
kernels/Conv_Winograd_v30_3_1_gfx11_fp32_f3x2_stride2.inc | ||
kernels/Conv_Winograd_v30_3_1_metadata.inc | ||
kernels/xform_bidirect_winograd_code.inc | ||
kernels/rocm_version.inc | ||
kernels/inst_wrappers.inc | ||
kernels/bfloat16_dev.hpp | ||
kernels/conv_common.inc | ||
kernels/utilities.inc | ||
kernels/xform_data_filter.inc | ||
kernels/xform_kd_cov2.inc | ||
kernels/xform_metadata.inc | ||
kernels/neuron.inc | ||
kernels/conv_sizes.inc | ||
kernels/gpr_alloc.inc | ||
kernels/bfloat16_dev.hpp | ||
kernels/float_types.h | ||
kernels/workaround_issue_1431.hpp | ||
kernels/gpr_alloc.inc | ||
kernels/hip_f8_impl.hpp | ||
kernels/hip_float8.hpp | ||
kernels/inst_wrappers.inc | ||
kernels/miopen_cstdint.hpp | ||
kernels/miopen_limits.hpp | ||
kernels/miopen_type_traits.hpp | ||
kernels/miopen_utility.hpp | ||
kernels/neuron.inc | ||
kernels/rocm_version.inc | ||
kernels/stride_array.hpp | ||
kernels/utilities.inc | ||
kernels/workaround_issue_1431.hpp | ||
kernels/xform_bidirect_winograd_code.inc | ||
kernels/xform_data_filter.inc | ||
kernels/xform_kd_cov2.inc | ||
kernels/xform_metadata.inc | ||
Comment on lines
-384
to
+403
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. Many changes due to sorting 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. Is there a convenient way to sort, or just
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. @CAHEK7 I use editor's function. |
||
) | ||
|
||
set(MIOPEN_KERNELS | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1292,8 +1292,7 @@ void BuildHip(const std::string& name, | |
auto opts = | ||
miopen::SplitSpaceSeparated(options, miopen::comgr::compiler::lc::GetOptionsNoSplit()); | ||
compiler::lc::RemoveOptionsUnwanted(opts); | ||
opts.push_back("-DWORKAROUND_ISSUE_HIPRTC_TRUE_TYPE"); // Workaround for SWDEV-308073 | ||
#if HIP_PACKAGE_VERSION_FLAT < 6000023494ULL | ||
#if HIP_PACKAGE_VERSION_MAJOR < 6 | ||
opts.push_back("-D__HIP_PLATFORM_HCC__=1"); // Workaround? | ||
#endif | ||
opts.push_back("-D__HIP_PLATFORM_AMD__=1"); // Workaround? | ||
|
@@ -1302,7 +1301,11 @@ void BuildHip(const std::string& name, | |
opts.push_back("-DCK_AMD_BUFFER_ATOMIC_FADD_RETURNS_FLOAT=1"); | ||
#endif | ||
opts.push_back("-DHIP_PACKAGE_VERSION_FLAT=" + std::to_string(HIP_PACKAGE_VERSION_FLAT)); | ||
opts.push_back("-DMIOPEN_DONT_USE_HIP_RUNTIME_HEADERS=1"); | ||
opts.push_back("-DMIOPEN_DONT_USE_HIP_RUNTIME_HEADERS"); | ||
/// For now, use only standard <limits> to avoid possibility of | ||
/// correctnes or performance regressions. | ||
/// \todo Test and enable "custom" local implementation. | ||
opts.push_back("-DWORKAROUND_DONT_USE_CUSTOM_LIMITS=1"); | ||
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. @atamazov it seems that we are ahead of compiler when trying to make changes into mainline. -- Build with HIP 6.0.23494 In ROCm 6.1 staging docker: -- Build with HIP 6.0.23464 and very strangely, we should not use custom limits (default here) in the newer compiler (first one above), but we still need to use it in the older one (second one above). 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. @junliume Are we having problems with the 6.0 release? This should not happen, i.e. it's a bug! What is the target GPU? You may have used some compilation path that was not tested on my Navi21. 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 all originated from this familiar issue:
So at least in HIP 6.0.23464 we cannot use the standard header, which we have worked around it by a customized header instead. It looks to me that some hipRTC changes were reverted in ROCm 6.0 branch to keep backward compatibility. However, these changes (reverts) were not propagated to mainline first. Thus we have a newer version of hipRTC with older behavior. This is problematic to us since we need to identify the applicability of workaround by the compiler/runtime version. In the code we can do it like:
We need the two precise points where this workaround should not be applicable. It's a WIP from my side. 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. @junliume We cannot rely on HIP versions reported by staging HIP. Those numbers are often very outdated. Therefore, basically we should not modify our code in order to adapt to the HIP versions from staging. UPDATE: If our code is correctly adapted for the upcoming HIP release, then it should be enough to use
This is wrong, because obviously current staging is newer than 6.0 release. Please use 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. @junliume And maybe we'll have to enable custom limits with staging HIP, but let's override HIP version first and see what happens. 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 above staging docker is "MIOpen/CK Staging docker" which is based on the latest mainline :) 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. @atamazov "let's override HIP version first and see what happens." any instructions how to do that? Overriding it with env vars does not work in my case by adding these to cmake command
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.
Yep, QA normally uses HIP Mainline to test MIOpen Staging (and promotes MIOpen to Mainline if passed, which seems reasonable) 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.
This is what cmake should print, please check: # CXX=/opt/rocm/llvm/bin/clang++ cmake ... -DMIOPEN_OVERRIDE_HIP_VERSION_MINOR=1 -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=0 ...
...
-- Build with HIP 6.0.23494
-- MIOPEN_hip_VERSION_MINOR overriden with 1
-- MIOPEN_hip_VERSION_PATCH overriden with 0
... Then you need to rebuild MIOpen. MIOpen should report HIP version 6.1.0 onto console. |
||
#if WORKAROUND_ISSUE_1431 | ||
if((StartsWith(target.Name(), "gfx10") || StartsWith(target.Name(), "gfx11")) && | ||
!miopen::comgr::IsWave64Enforced(opts)) | ||
|
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.
@CAHEK7