Skip to content
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

[NFC] Moved DetectRocm calls to constructor of ExecutionContext #2295

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

DrizztDoUrden
Copy link
Contributor

Implements #2289

averinevg
averinevg previously approved these changes Aug 2, 2023
Copy link
Contributor

@averinevg averinevg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@junliume
Copy link
Contributor

junliume commented Aug 7, 2023

Hi @DrizztDoUrden this is triggering some strange issues in CI:

[2023-08-02T21:03:43.755Z] /var/jenkins/workspace/s_MIOpen_ddu_private-detect-rocm/fin/src/include/tensor.hpp:30:5: warning: 'HIP_PACKAGE_VERSION_FLAT' is not defined, evaluates to 0 [-Wundef]

[2023-08-02T21:03:43.755Z] #if HIP_PACKAGE_VERSION_FLAT >= 5006000000ULL

[2023-08-02T21:03:43.755Z]     ^

[2023-08-02T21:03:47.689Z] In file included from /var/jenkins/workspace/s_MIOpen_ddu_private-detect-rocm/fin/src/main.cpp:29:

[2023-08-02T21:03:47.689Z] /var/jenkins/workspace/s_MIOpen_ddu_private-detect-rocm/fin/src/include/conv_fin.hpp:211:9: error: 'DetectRocm' is a private member of 'miopen::ExecutionContext'

[2023-08-02T21:03:47.689Z]     ctx.DetectRocm();

[2023-08-02T21:03:47.689Z]         ^

If HIP_PACKAGE_VERSION_FLAT is not defined in the scope, why it passed previous CI runs?

@atamazov
Copy link
Contributor

atamazov commented Aug 7, 2023

@junliume

If HIP_PACKAGE_VERSION_FLAT is not defined in the scope, why it passed previous CI runs?

Because this is just a warning.

Note that this happens during fin build. IIRC we do not use fin in our tidy checks; fin has its own Jenkinsfile (and its own tidy check). Why it does not catch the problem like this is another story. <config.h> must be included before using HIP_PACKAGE_VERSION_FLAT in fin/src/include/tensor.hpp. I highly recommend fixing both issues in fin. @cderb @JehandadKhan

@cderb
Copy link
Contributor

cderb commented Aug 7, 2023

I'm looking into resolving the warning from HIP_PACKAGE_VERSION_FLAT.
It looks like moving DetectRocm caused some breakages in fin as well. It appears that the solution is to simply remove these lines?

@atamazov
Copy link
Contributor

atamazov commented Aug 7, 2023

@cderb Exactly. What is the process of updating both fin and MIOpen? It seems like we can't commit changes in fin without prior changes in MIOpen and vice versa 🤔

@cderb
Copy link
Contributor

cderb commented Aug 7, 2023

@atamazov this time lets change the fin branch here, then we can redirect it back to develop once the repos are aligned.
If you use this fin branch for the submodule, I believe we can pass the CI: cerb/detect_rocm_private

@atamazov
Copy link
Contributor

atamazov commented Aug 7, 2023

@DrizztDoUrden FYI 👆

Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@junliume junliume merged commit a0c73f5 into develop Aug 10, 2023
@junliume junliume deleted the ddu/private-detect-rocm branch August 28, 2023 23:40
junliume added a commit that referenced this pull request Sep 6, 2023
junliume added a commit that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants