Skip to content

Allow targetid for mcpu kernel arguments#3699

Closed
cderb wants to merge 14 commits into
developfrom
cderb/kdb_targetid
Closed

Allow targetid for mcpu kernel arguments#3699
cderb wants to merge 14 commits into
developfrom
cderb/kdb_targetid

Conversation

@cderb
Copy link
Copy Markdown
Contributor

@cderb cderb commented Apr 25, 2025

Resolve issue for generating system kdb #3001
unblock #2891 resolving #2851

Comment thread test/gtest/db_sync.cpp Outdated
Comment on lines +530 to +534
// Probably, according to the idea of the author of this test, the number of CUs should have been
// substituted with the value passed to the constructor (which in fact did not happen). After
// https://github.com/ROCm/MIOpen/pull/3175, the method became virtual, the substitution actually
// happened, and the test broke. I disabled that part (since it doesn't work as intended anyway) to
// keep its behavior the same.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment and the #if/#endif can be removed. I wrote the comment and disabled the code below, but now it is enabled again, someone probably fixed it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the comment and #defs.

#include <string>

#define WORKAROUND_ISSUE_3001 1
#define WORKAROUND_ISSUE_3001 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I think so.

Copy link
Copy Markdown
Contributor

@BrianHarrisonAMD BrianHarrisonAMD left a comment

Choose a reason for hiding this comment

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

LGTM, but probably need to update, and re-run CI.

@amd-hsivasun
Copy link
Copy Markdown

Imported to ROCm/rocm-libraries

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.

4 participants