[HIP][COMGR] Use COMGR by default for HIP backend #1253

atamazov · 2021-10-29T22:22:14Z

Enable COMGR by default for HIP backend, except NOGPU mode (for Tuna)
[Jenkinsfile] Exchange COMGR stages with NOCOMGR stages. Remove W/A for SWDEV-290754 (fixed in 4.3)
WORKAROUND_ISSUE_1257 disable COMGR build for gfx90a, FP16 for [gfx90a][COMGR][Fp16][Bf16] Verification errors of ImplicitGemm kernels (no errors with hip-clang) #1257
Some tidy fixes for COMGR path.

…PU mode (for Tuna)

…ages (to smoke test offline builds). Removed W/A for SWDEV-290754 (fixed in 4.3)

…a, FP16

# RESOLVED Conflicts: # Jenkinsfile

This reverts commit ac70214.

atamazov · 2021-11-03T21:08:29Z

Passed all CI checks and ready for review.

junliume · 2021-11-04T06:14:01Z

Gentle ping for @pfultz2 @JehandadKhan and @jerryyin for review.

atamazov · 2021-11-04T23:02:48Z

⚠️ This PR would lead to gfx90a failures during staging testing

...Because workaround for #1257 is implemented in Jenkinsfile, but CQE use their own. W/A should be re-implemented in CMake. It should use GPU detection + switching off COMGR if it is gfx90a.

@junliume How much time do we have?

atamazov · 2021-11-04T23:04:14Z

Oh no, the above is not possible because they build dockers somewhere else IIRC...

atamazov · 2021-11-04T23:06:19Z

It seems like the only option is disable the failing tests for (gfx90a && comgr), dynamically.

junliume · 2021-11-05T02:59:01Z

It seems like the only option is disable the failing tests for (gfx90a && comgr), dynamically.

@atamazov Staging tests do not include gfx90a unless we ask explicitly. We can explicitly ask them to disable the failing tests for (gfx90a && comgr).
As for the time, I think we still have a few weeks till branching. #1258 definitely have higher priority than this one.

atamazov · 2021-11-08T22:29:09Z

CMakeLists.txt

+set_var_to_condition(MIOPEN_USE_COMGR_DEFAULT (NOT DEFINED MIOPEN_BACKEND_OPENCL) AND (NOT DEFINED MIOPEN_MODE_NOGPU))
+option(MIOPEN_USE_COMGR "Use comgr to build kernels instead of offline tools" ${MIOPEN_USE_COMGR_DEFAULT})


…r HIP backend (#1253)" This reverts commit c0636c7.

). Add regression test for issue-internal #4 (#1273) Resolves (hides symptoms of) https://github.com/ROCmSoftwarePlatform/MIOpen-internal/issues/4 and #1257. - Reverted #1253 - Kept some useful stuff for CMake + tidy fixes for 4.5 - Added regression test for https://github.com/ROCmSoftwarePlatform/MIOpen-internal/issues/4 [Informative] Actual problem is that kernels built via COMGR may have correctness problems (we know some cases for gfx90a).

atamazov · 2021-11-11T13:53:02Z

Jenkinsfile

+    // WORKAROUND_SWDEV_290754
+    // It seems like this W/A is not required since 4.5.
+    def build_cmd = conf.get("build_cmd", "LLVM_PATH=/opt/rocm/llvm ${build_envs} dumb-init make -j\$(nproc) ${config_targets}")


… by default for HIP backend (revert #1253).." Keep regression test for issue-internal #4 This reverts commit edd87ed.

…om COMGR by default for HIP backend (revert #1253).." Keep regression test for issue-internal #4" This reverts commit 14d6ab0. # RESOLVED Conflicts: # Jenkinsfile

atamazov · 2022-12-14T21:51:13Z

CMakeLists.txt

+    if(${ARGN})
+        set(${var} TRUE)
+    else()
+        set(${name} FALSE)


This is wrong, ${var} should be used here. No problem so far because unset var is evaluated to FALSE in conditionals, and we do not use set_var_to_condition to change values of variables. To be fixed soon.

…, see ROCm#1253 (review)

atamazov added 2 commits October 30, 2021 01:06

comgr-default(01) Enable COMGR by default for HIP backend, except NOG…

a595347

…PU mode (for Tuna)

comgr-default(02) [Jenkinsfile] Exchanged COMGR stages for NOCOMGR st…

5aecddb

…ages (to smoke test offline builds). Removed W/A for SWDEV-290754 (fixed in 4.3)

atamazov added the urgency_high label Oct 29, 2021

atamazov added this to the ROCm 5.0 milestone Oct 29, 2021

atamazov requested review from pfultz2, jerryyin, JehandadKhan and junliume October 29, 2021 22:22

atamazov changed the title ~~[HIP][COMGR] COMGR by default for HIP backend~~ [HIP][COMGR] Use COMGR by default for HIP backend Oct 29, 2021

atamazov added 4 commits October 30, 2021 02:10

comgr-default(03) Reintroduce WORKAROUND_SWDEV_290754.

737dc03

comgr-default(04) [TEMP][CI] disable static checks & packaging

ac70214

comgr-default(05) [COMGR] Tidy fixes

ceb318a

comgr-default(06) WORKAROUND_ISSUE_1275 disable COMGR build for gfx90…

f2a8db5

…a, FP16

This comment has been minimized.

Sign in to view

atamazov added 5 commits November 2, 2021 21:35

Added WORKAROUND_SWDEV_290754 for Navi21.

5a3e1d5

Typo

0eb0593

Fix syntax errors

594fe4f

Merge branch 'develop' into comgr-default

2e73126

# RESOLVED Conflicts: # Jenkinsfile

Revert "comgr-default(04) [TEMP][CI] disable static checks & packaging"

4450c12

This reverts commit ac70214.

atamazov marked this pull request as ready for review November 3, 2021 20:51

junliume added the TESTING_CI_PASSED label Nov 4, 2021

JehandadKhan approved these changes Nov 4, 2021

View reviewed changes

jerryyin approved these changes Nov 4, 2021

View reviewed changes

junliume approved these changes Nov 4, 2021

View reviewed changes

junliume merged commit c0636c7 into develop Nov 4, 2021

atamazov commented Nov 8, 2021

View reviewed changes

atamazov mentioned this pull request Nov 9, 2021

[gfx90a][COMGR][Fp16][Bf16] Verification errors of ImplicitGemm kernels (no errors with hip-clang) #1257

Open

atamazov added a commit that referenced this pull request Nov 9, 2021

revert-comgr-default(01) Revert "[HIP][COMGR] Use COMGR by default fo…

71ed87e

…r HIP backend (#1253)" This reverts commit c0636c7.

atamazov added a commit that referenced this pull request Nov 9, 2021

revert-comgr-default(02) Restore some useful stuff from #1253

3f71ba2

atamazov mentioned this pull request Nov 9, 2021

[HIP][COMGR] Abstain from COMGR by default for HIP backend (revert #1253). Add regression test for issue-internal #4 #1273

Merged

atamazov commented Nov 11, 2021

View reviewed changes

atamazov added a commit that referenced this pull request Nov 13, 2021

comgr-default-2(01) Partially revert "[HIP][COMGR] Abstain from COMGR…

14d6ab0

… by default for HIP backend (revert #1253).." Keep regression test for issue-internal #4 This reverts commit edd87ed.

atamazov deleted the comgr-default branch December 6, 2021 13:31

atamazov commented Dec 14, 2022

View reviewed changes

atamazov added a commit to atamazov/MIOpen that referenced this pull request Dec 14, 2022

ata-test-solver-03(24) [CMake][Fix] Fix issue in set_var_to_condition…

a8eca64

…, see ROCm#1253 (review)

atamazov mentioned this pull request Dec 19, 2022

[tests] Added smoke tests for conv solvers (ConvAsmBwdWrW1x1 and more). Some fixes. #1906

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HIP][COMGR] Use COMGR by default for HIP backend #1253

[HIP][COMGR] Use COMGR by default for HIP backend #1253

atamazov commented Oct 29, 2021 •

edited

Loading

This comment has been minimized.

atamazov commented Nov 3, 2021

junliume commented Nov 4, 2021

atamazov commented Nov 4, 2021

atamazov commented Nov 4, 2021

atamazov commented Nov 4, 2021

junliume commented Nov 5, 2021

atamazov Nov 8, 2021

atamazov Nov 11, 2021

atamazov Dec 14, 2022

		set_var_to_condition(MIOPEN_USE_COMGR_DEFAULT (NOT DEFINED MIOPEN_BACKEND_OPENCL) AND (NOT DEFINED MIOPEN_MODE_NOGPU))
		option(MIOPEN_USE_COMGR "Use comgr to build kernels instead of offline tools" ${MIOPEN_USE_COMGR_DEFAULT})

[HIP][COMGR] Use COMGR by default for HIP backend #1253

[HIP][COMGR] Use COMGR by default for HIP backend #1253

Conversation

atamazov commented Oct 29, 2021 • edited Loading

This comment has been minimized.

atamazov commented Nov 3, 2021

junliume commented Nov 4, 2021

atamazov commented Nov 4, 2021

⚠️ This PR would lead to gfx90a failures during staging testing

atamazov commented Nov 4, 2021

atamazov commented Nov 4, 2021

junliume commented Nov 5, 2021

atamazov Nov 8, 2021

Choose a reason for hiding this comment

atamazov Nov 11, 2021

Choose a reason for hiding this comment

atamazov Dec 14, 2022

Choose a reason for hiding this comment

atamazov commented Oct 29, 2021 •

edited

Loading