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.

Sign up for GitHub

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

Jump to bottom

[MLIR][OpenMP][OMPIRBuilder] Support omp.target 'if' translation to LLVM IR #157

Merged
merged 1 commit into from
Sep 19, 2024
Merged

[MLIR][OpenMP][OMPIRBuilder] Support omp.target 'if' translation to LLVM IR #157

merged 1 commit into from
Sep 19, 2024

Conversation

skatrak
Copy link

@skatrak skatrak commented Sep 17, 2024

This patch adds missing MLIR to LLVM IR translation support for the if clause on omp.target operations. This completes the missing piece for Fortran support of !$omp target if(...).

The implementation updates emitTargetCall() in the OMPIRBuilder to follow clang's support for the if clause in CGOpenMPRuntime::emitTargetCall().

@skatrak
[MLIR][OpenMP][OMPIRBuilder] Support omp.target IF translation to LLV…
90560e7 
…M IR

This patch adds missing MLIR to LLVM IR translation support for the `if` clause
on `omp.target` operations. This completes the missing piece for Fortran
support of `!$omp target if(...)`.

The implementation updates `emitTargetCall()` in the OMPIRBuilder to follow
clang's support for the `if` clause in  `CGOpenMPRuntime::emitTargetCall()`.
mjklemm
mjklemm approved these changes Sep 18, 2024
View reviewed changes
Copy link

@mjklemm mjklemm 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 I have had a hard time following the changes as the diff was sort of messed up. So, second pair of eyes might be useful.

@skatrak
Copy link
Author

skatrak commented Sep 18, 2024

LGTM, but I have had a hard time following the changes as the diff was sort of messed up. So, second pair of eyes might be useful.

Yes, it flags a bunch of changes because I basically extracted most of the body of emitTargetCall into a couple of lambdas. The only real change is at the bottom of the function, where it's decided when to run each part. It's generally a simple change: just adding an if-else depending on the runtime value of the if expression, if present, otherwise it does the same thing it used to.

dpalermo
dpalermo approved these changes Sep 18, 2024
View reviewed changes
Copy link

@dpalermo dpalermo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Verified this changes works correctly on Leopold's reproducer.

@bhandarkar-pranav
Copy link

@skatrak thank you for the PR. LGTM.

@skatrak skatrak merged commit 748aa57 into ROCm:amd-trunk-dev Sep 19, 2024
3 of 5 checks passed
@skatrak skatrak deleted the target-if branch September 19, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpalermo dpalermo dpalermo approved these changes

@mjklemm mjklemm mjklemm approved these changes

@jsjodin jsjodin Awaiting requested review from jsjodin

@agozillon agozillon Awaiting requested review from agozillon

@DominikAdamski DominikAdamski Awaiting requested review from DominikAdamski

@bhandarkar-pranav bhandarkar-pranav Awaiting requested review from bhandarkar-pranav

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skatrak @bhandarkar-pranav @mjklemm @dpalermo