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

[Flang][MLIR][OpenMP] Fix num_teams, num_threads, thread_limit lowering #132

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

skatrak
Copy link

@skatrak skatrak commented Aug 5, 2024

This patch fixes lowering for the num_teams, num_threads and thread_limit clauses when inside of a target region and compiling for the host device.

The current approach requires these to be attached to the parent MLIR omp.target operation. However, some incorrect checks based on the evalHasSiblings() helper function would result in these clauses being attached to the omp.teams or omp.parallel operation instead, triggering a verifier error.

In this patch, these checks are updated to stop breaking when lowering combined target teams [X] constructs. Also, the genTeamsClauses() function is fixed to avoid processing num_teams and thread_limit twice, which probably resulted from a recent merge.

Copy link

@agozillon agozillon left a comment

Choose a reason for hiding this comment

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

LGTM! Might be worth throwing in a really simple test if we have one on hand just so we pick up if it gets broken in a future merge, but I'll leave that and the nit (if it's possible) up to you! :-)

flang/lib/Lower/OpenMP/OpenMP.cpp Outdated Show resolved Hide resolved
This patch fixes lowering for the num_teams, num_threads and thread_limit
clauses when inside of a target region and compiling for the host device.

The current approach requires these to be attached to the parent MLIR
omp.target operation. However, some incorrect checks based on the
`evalHasSiblings()` helper function would result in these clauses being
attached to the `omp.teams` or `omp.parallel` operation instead, triggering
a verifier error.

In this patch, these checks are updated to stop breaking when lowering
combined `target teams [X]` constructs. Also, the `genTeamsClauses()` function
is fixed to avoid processing num_teams and thread_limit twice, which probably
resulted from a recent merge.
@skatrak
Copy link
Author

skatrak commented Aug 8, 2024

Thank you @agozillon for your suggestions. When adding a set of tests I realized that verifier restrictions were too restrictive and that I can't think of a good generic way of identifying in MLIR the cases where these arguments should be attached to the parent omp.target. So I'm removing these checks and leaving it to the MLIR creation stage to make that decision.

@skatrak skatrak merged commit 4f5f002 into ROCm:amd-trunk-dev Aug 9, 2024
2 of 4 checks passed
@skatrak skatrak deleted the num-teams-threads-limit branch August 9, 2024 12:50
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.

2 participants