[CI] Update CI docker and suppress warnings #2333

junliume · 2023-08-19T00:09:54Z

No description provided.

junliume · 2023-08-20T07:01:25Z

@JehandadKhan @atamazov
We have seen many issues with this update and some of them look like:

[2023-08-20T06:24:00.398Z] /long_pathname_so_that_rpms_can_package_the_debug_info/src/out/ubuntu-20.04/20.04/build/hip-on-rocclr/hipamd/src/hiprtc/hip_rtc_gen/hipRTC_header.h:1034:49: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
[2023-08-20T06:24:00.398Z]       __hip_assert(false && "invalid tile size");
[2023-08-20T06:24:00.398Z]                                                 ^

another issue:

[2023-08-20T06:24:00.885Z] MIOpen(HIP): Info [AmdRocmMetadataVersionDetect] ROCm MD version AMDHSA_COv3, HIP version 5.7.23302, MIOpen version 3.00.0.a9f822391
[2023-08-20T06:24:00.885Z] MIOpen(HIP): Info [PrintVersionImpl] COMgr v.2.5.0, USE_HIP_PCH: 1
[2023-08-20T06:24:00.885Z] /home/jenkins/workspace/MLLibs_MIOpen_ci_rocm57/src/include/miopen/hipoc_kernel.hpp:61:36: runtime error: constructor call on misaligned address 0x7fff9a5b698c for type 'const void *', which requires 8 byte alignment
[2023-08-20T06:24:00.885Z] 0x7fff9a5b698c: note: pointer points here

which might be sanitizer checks related. by making this change

static const int second_index = (sizeof(T) + padding + 7) & ~7;

is not helping unfortunately.

atamazov · 2023-08-21T15:50:20Z

@junliume BTW I see that submodule fin updated from ebf9b355da09e10cb77e4b163a02e57be954095e to b2f3f4db3c3d7dd757e6d9e68719a780d8114dfa. Diff seems empty. Please revert this change.

cderb · 2023-08-21T16:10:24Z

I see that submodule fin updated from ebf9b355da09e10cb77e4b163a02e57be954095e to b2f3f4db3c3d7dd757e6d9e68719a780d8114dfa. Diff seems empty. Please revert this change.

This is ok @atamazov. The branch update returns Fin to the tip of fin develop after it was branched for PR #2295.

cderb · 2023-08-21T16:13:35Z

We have seen many issues with this update and some of them look like:

@junliume I've been addressing many of the rocm 5.7 related make check issues in an MIOpen-internal branch, I'll send you.

atamazov · 2023-08-21T16:23:04Z

@junliume

@JehandadKhan @atamazov We have seen many issues with this update and some of them look like:

[2023-08-20T06:24:00.398Z] /long_pathname_so_that_rpms_can_package_the_debug_info/src/out/ubuntu-20.04/20.04/build/hip-on-rocclr/hipamd/src/hiprtc/hip_rtc_gen/hipRTC_header.h:1034:49: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
[2023-08-20T06:24:00.398Z]       __hip_assert(false && "invalid tile size");
[2023-08-20T06:24:00.398Z]                                                 ^

This seems like a a problem in the implementation of the precompiled HIP headers. I do not know what __hip_assert is, but I suspect that it should not expand to nothing when NDEBUG is defined. If I am right, then the simplest way to fix this in the HIP compiler/library is to define that macro like this state-of-the-art thing:

#ifndef NDEBUG
#define __hip_assert(ex) ... // normal definition of assert
#else
#define __hip_assert(ex) (void(0))
#endif

The code like (void(0)); should not produce any warning. Alternatively, hipRTC_header.h should observe NDEBUG at line 1034 and do not use __hip_assert when NDEBUG is defined.

Again, AFAICS this happens in the PCH, not in our code. I recommend informing the guys who are responsible for PCH, then silence the warning in MIOpen permanently (I do not think it's worth a full-featured "workaround" stuff) and forgetting about this.

JehandadKhan · 2023-08-21T16:28:49Z

@junliume
@JehandadKhan @atamazov We have seen many issues with this update and some of them look like:
[2023-08-20T06:24:00.398Z] /long_pathname_so_that_rpms_can_package_the_debug_info/src/out/ubuntu-20.04/20.04/build/hip-on-rocclr/hipamd/src/hiprtc/hip_rtc_gen/hipRTC_header.h:1034:49: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
[2023-08-20T06:24:00.398Z]       __hip_assert(false && "invalid tile size");
[2023-08-20T06:24:00.398Z]                                                 ^
This seems like a a problem in the implementation of the precompiled HIP headers. I do not know what __hip_assert is, but I suspect that it should not expand to nothing when NDEBUG is defined. If I am right, then the simplest way to fix this in the HIP compiler/library is to define that macro like this state-of-the-art thing:
#ifndef NDEBUG
#define __hip_assert(ex) ... // normal definition of assert
#else
#define __hip_assert(ex) (void(0))
#endif
The code like (void(0)); should not produce any warning. Alternatively, hipRTC_header.h should observe NDEBUG at line 1034 and do not use __hip_assert when NDEBUG is defined.

Again, AFAICS this happens in the PCH, not in our code. I recommend informing the guys who are responsible for PCH, then silence the warning in MIOpen permanently (I do not think it's worth a full-featured "workaround" stuff) and forgetting about this.

@atamazov I agree, this should be fixed in the runtime headers

junliume · 2023-08-21T16:29:19Z

@junliume BTW I see that submodule fin updated from ebf9b355da09e10cb77e4b163a02e57be954095e to b2f3f4db3c3d7dd757e6d9e68719a780d8114dfa. Diff seems empty. Please revert this change.

This is to keep track of FIN's develop branch. I see this happens several times:

When updating MIOpen, FIN commit hash is updated to a "branch" commit hash,
When that branch commit hash is committed to FIN, there will be no difference between its develop branch and the one used to commit in MIOpen.

I think we need a standard procedure to update FIN :) notably, Clang formatting will change one file in FIN and I wanted to make that change a while ago.

src/include/miopen/hipoc_kernel.hpp

atamazov · 2023-08-22T23:09:27Z

src/include/miopen/hipoc_kernel.hpp

        new(buffer) T(x); // NOLINT (clang-analyzer-cplusplus.PlacementNew)
        new(buffer + second_index) U(y);
    }
-    char buffer[second_index + sizeof(U)] = {};
+    alignas(U) char buffer[second_index + sizeof(U)] = {};


This may change the layout of kernel arguments in memory. Without alignas(U), the required buffer alignment is 1 (i.e., no alignment is required). ~~Therefore, the alignment of KernelArgsPair instances is also 1, and several such instances reside in memory without any gaps.~~

With alignas(U), the padding required for KernelArgsPair is alginof(U)~~, which may lead to gaps between instances of KernelArgsPair.~~

If you see kernel failures, then please revert this change.

@atamazov I do see lots of failures on gfx906 with

handlehip.cpp:80: Failed getting available memory: invalid argument

However, it looks more likely a runtime issue since it cannot be reproduced on other newer ASICs. But I will revert this change and try again.

@junliume This is not related and I see the same on navi21.

Reverting this change won't resolve the issue with getting available memory.

@junliume It seems like this change is indeed correct and should be kept.

@atamazov can you verify it happens on Navi21?
For Vega nodes some have problems but some other nodes do not.

@junliume Of course, this happens on Navi21, as I've reported a while ago at #2307 (comment) where you can find the dirty hacks for this. I am working on more or less regular W/A which should be suitable for merging into develop.

atamazov · 2023-08-23T14:08:26Z

@junliume I am working on this. Please expect update soon.

…age. Fix: Removed unnecessary params.TARGET_NAVI21 from AnyGPU stages.

# RESOLVED Conflicts: # requirements.txt

atamazov · 2023-08-24T22:54:34Z

@junliume I think that https://github.com/atamazov/MIOpen/blob/ci_rocm57_ata1 is ready for merging here.

Fixed potential UB in KernelArgs detected by sanitizer.
WORKAROUND_FAULTY_HIPMEMGETINFO_VEGA_NAVI2X introduced.
- If hipMemGetInfo fails, issues warning message and returns "fake" 16 GiB value. For now, this allows for the continued use of gfx906 and Nav21 nodes in development and testing.
- W/A is GPU-agnostic because we do not know the GPU type in the place where the W/A is applied.
- W/A may be considered as potentially dangerous because it always camouflages hipMemGetInfo failures. However, we're safe as long as hipMemGetInfo works fine (as it should on all officially supported GPUs, as the HIP runtime must be well tested before use with MIOpen).
[CI] Fix: Applied WORKAROUND_ISSUE_1148 to a 'Fp16 Hip All gfx1030' stage.
- This is expected to prevent random failures on Navi21 nodes. My Navi21 system fails quite consistently (but randomly ;) without this W/A.
[CI] Quality: Removed unnecessary params.TARGET_NAVI21 from AnyGPU stages.
Merged recent changes from ci_rocm57, except b59403d
Merged recent changes from develop, conflicts resolved.

🌀 Testing

Tested on Navi21 and gfx906/60 (Radeon VII) systems. Note that my Navi21 system has the "hipMemGetInfo" issue, but my Radeon VII doesn't. Tried the followed tests on both systems:

Debug build with sanitizers, Smoke FP32 test - PASSED
Release build, Full FP16 test - PASSED

atamazov · 2023-08-24T23:03:19Z

@junliume b59403d is merged into https://github.com/atamazov/MIOpen/blob/ci_rocm57_ata1.

junliume · 2023-08-25T20:15:11Z

WORKAROUND_FAULTY_HIPMEMGETINFO_VEGA_NAVI2X

@atamazov I am not reproducing the hipGetMemoryInfo() error with 5.7 RC3 docker (at least so far), but indeed many CI nodes are not as stable now, and I have asked these base OS to be updated.

atamazov · 2023-08-25T21:01:24Z

@junliume Are you going to merge https://github.com/atamazov/MIOpen/blob/ci_rocm57_ata1 into this branch? If yes, then I will double-check if hipMemGetInfo problem is resolved in RC3 and narrow (or remove) the W/A.

junliume · 2023-08-25T21:19:37Z

@junliume Are you going to merge https://github.com/atamazov/MIOpen/blob/ci_rocm57_ata1 into this branch? If yes, then I will double-check if hipMemGetInfo problem is resolved in RC3 and narrow (or remove) the W/A.

I wish to and need to figure out how to sync from ci_rocm57_ata1 ...

Update: done.

atamazov · 2023-08-26T20:47:22Z

@junliume hipMemGetInfo problem is NOT resolved in 5.7 RC3. This PR can be merged as is.

atamazov · 2023-08-26T21:33:19Z

@junliume @JehandadKhan [Notice] Interesting detail:
Both RC1 and RC3 report HIP version 5.7.31921 in hip_version.h which is not good. However, CMake (hip-config-version.cmake) reports different numbers:

RC1: 5.7.23302
RC3: 5.7.23341

"Thanks" to some problems with hip_version.h in some old versions of ROCm we use CMake machinery as a workaround, and the library is able to distinguish between RC1 and RC3 (even if this is not necessary yet).

junliume · 2023-08-28T21:10:15Z

Ping @JehandadKhan and @atamazov for review after CI has passed.

Update CI docker to ROCm 5.7 RC2

b600234

junliume added urgency_normal value_unknown labels Aug 19, 2023

junliume added this to the ROCm 6.0 milestone Aug 19, 2023

junliume added urgency_blocker and removed urgency_normal labels Aug 19, 2023

junliume added 6 commits August 18, 2023 21:03

fix issue with repo

bcf4fcb

update docker build arch list

c6be2b2

Update CK commit hash

0f9c9e3

update FIN to its latest develop

3d061fe

revert CK commit hash

f779a67

revert CK commit

a9f8223

junliume mentioned this pull request Aug 20, 2023

[BUG] compile errors with HIPRTC #1648

Closed

junliume added 2 commits August 20, 2023 00:12

suppress warning from hipRTC

70c50d8

suppress UNSAN

0da7bc3

fix the alignment issue

83a19ed

atamazov reviewed Aug 21, 2023

View reviewed changes

src/include/miopen/hipoc_kernel.hpp Outdated Show resolved Hide resolved

fix alignment issue

109d623

atamazov reviewed Aug 22, 2023

View reviewed changes

junliume added urgency_high and removed urgency_blocker labels Aug 22, 2023

revert an alignment change to check gfx906 CI

dd72b0d

atamazov and others added 7 commits August 23, 2023 18:29

Fix potential UB in KernelArgs detected by sanitizer.

4d1b82a

Update dependencies CK and Half

d0b7c14

WORKAROUND_FAULTY_HIPMEMGETINFO_VEGA_NAVI2X

6477e68

[CI] Fix: Applid WORKAROUND_ISSUE_1148 to a 'Fp16 Hip All gfx1030' st…

2120a23

…age. Fix: Removed unnecessary params.TARGET_NAVI21 from AnyGPU stages.

Merge branch 'ci_rocm57' into ci_rocm57_ata1

93d36ee

Avoid log clogs and find half include header

b59403d

Merge branch 'develop' into ci_rocm57_ata1

4643c44

# RESOLVED Conflicts: # requirements.txt

Merge branch 'ci_rocm57' into ci_rocm57_ata1

b268f40

Update ROCm to 5.7 RC3

47d34a4

Merge branch 'develop' into ci_rocm57

dc3c013

Merge branch 'ci_rocm57_ata1' into ci_rocm57

68e106e

junliume added the TESTING_CI_PASSED label Aug 28, 2023

junliume requested a review from JehandadKhan August 28, 2023 21:09

JehandadKhan approved these changes Aug 28, 2023

View reviewed changes

junliume merged commit b85c7c3 into develop Aug 28, 2023

junliume deleted the ci_rocm57 branch February 6, 2024 20:10

atamazov mentioned this pull request Aug 10, 2024

[CI][Failure] shared_ptr_base.h:199:9: runtime error: member call on address which does not point to an object of type 'std::_Sp_counted_base<>' #3192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Update CI docker and suppress warnings #2333

[CI] Update CI docker and suppress warnings #2333

junliume commented Aug 19, 2023

junliume commented Aug 20, 2023 •

edited

Loading

atamazov commented Aug 21, 2023

cderb commented Aug 21, 2023

cderb commented Aug 21, 2023

atamazov commented Aug 21, 2023

JehandadKhan commented Aug 21, 2023

junliume commented Aug 21, 2023

atamazov Aug 22, 2023 •

edited

Loading

junliume Aug 22, 2023

atamazov Aug 22, 2023

atamazov Aug 23, 2023

junliume Aug 23, 2023

atamazov Aug 24, 2023

atamazov commented Aug 23, 2023

atamazov commented Aug 24, 2023 •

edited

Loading

atamazov commented Aug 24, 2023

junliume commented Aug 25, 2023

atamazov commented Aug 25, 2023

junliume commented Aug 25, 2023 •

edited

Loading

atamazov commented Aug 26, 2023

atamazov commented Aug 26, 2023

junliume commented Aug 28, 2023

[CI] Update CI docker and suppress warnings #2333

[CI] Update CI docker and suppress warnings #2333

Conversation

junliume commented Aug 19, 2023

junliume commented Aug 20, 2023 • edited Loading

atamazov commented Aug 21, 2023

cderb commented Aug 21, 2023

cderb commented Aug 21, 2023

atamazov commented Aug 21, 2023

JehandadKhan commented Aug 21, 2023

junliume commented Aug 21, 2023

atamazov Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

junliume Aug 22, 2023

Choose a reason for hiding this comment

atamazov Aug 22, 2023

Choose a reason for hiding this comment

atamazov Aug 23, 2023

Choose a reason for hiding this comment

junliume Aug 23, 2023

Choose a reason for hiding this comment

atamazov Aug 24, 2023

Choose a reason for hiding this comment

atamazov commented Aug 23, 2023

atamazov commented Aug 24, 2023 • edited Loading

🌀 Testing

atamazov commented Aug 24, 2023

junliume commented Aug 25, 2023

atamazov commented Aug 25, 2023

junliume commented Aug 25, 2023 • edited Loading

atamazov commented Aug 26, 2023

atamazov commented Aug 26, 2023

junliume commented Aug 28, 2023

junliume commented Aug 20, 2023 •

edited

Loading

atamazov Aug 22, 2023 •

edited

Loading

atamazov commented Aug 24, 2023 •

edited

Loading

junliume commented Aug 25, 2023 •

edited

Loading