Skip to content

Remove OpenMP::omp imported target from rocwmma#419

Merged
estewart08 merged 1 commit intoamd-stagingfrom
amd/dev/estewart/remove-omp-target-rocwmma
Mar 11, 2026
Merged

Remove OpenMP::omp imported target from rocwmma#419
estewart08 merged 1 commit intoamd-stagingfrom
amd/dev/estewart/remove-omp-target-rocwmma

Conversation

@estewart08
Copy link
Copy Markdown
Contributor

Recently rocwmma switched to using the openmp-config in ROCm, which provides the OpenMP::omp target. I see that the OpenMP::OpenMP_CXX target was removed before so we are just adding to that logic. That being said I think removing these targets is hacky and rocwmma should instead use add_dependency in their config to resolve/lookup these targets.

@estewart08 estewart08 requested a review from a team as a code owner February 18, 2026 15:56
@zichguan-amd
Copy link
Copy Markdown
Collaborator

@adeljo-amd can you review and sync #411 if necessary? Thanks

@adeljo-amd
Copy link
Copy Markdown
Contributor

@adeljo-amd can you review and sync #411 if necessary? Thanks

Oops sorry, my email notifications was a mess and I didn't see this before merging in 411. This LGTM, though I agree about the removal of the target being hacky

@zichguan-amd
Copy link
Copy Markdown
Collaborator

@estewart08 can you also include the newly added perf_i8gemm? Otherwise LGTM

Recently rocwmma switched to using the openmp-config in ROCm,
which provides the OpenMP::omp target. I see that the OpenMP::OpenMP_CXX target
was removed before so we are just adding to that logic. That being said
I think removing these targets is hacky and rocwmma should instead
use add_dependency in their config to resolve/lookup these targets.
@estewart08 estewart08 force-pushed the amd/dev/estewart/remove-omp-target-rocwmma branch from 7d1f4a3 to 63c6b83 Compare March 9, 2026 17:53
@estewart08
Copy link
Copy Markdown
Contributor Author

@estewart08 can you also include the newly added perf_i8gemm? Otherwise LGTM

Done.

Copy link
Copy Markdown
Collaborator

@zichguan-amd zichguan-amd left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@estewart08
Copy link
Copy Markdown
Contributor Author

@zichguan-amd Can you comment on the failing checks? From what I can tell they are unrelated to the patch.

@zichguan-amd
Copy link
Copy Markdown
Collaborator

@estewart08 the rocprofiler-sdk failures are unrelated, you can go ahead and merge this. Thanks.

@estewart08 estewart08 merged commit c74714c into amd-staging Mar 11, 2026
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants