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

[OpenMP] [OMPT] A pointer to HostOpId should be passed in EMI callbacks. #75574

Merged
merged 1 commit into from
Dec 15, 2023
Merged

[OpenMP] [OMPT] A pointer to HostOpId should be passed in EMI callbacks. #75574

merged 1 commit into from
Dec 15, 2023

Conversation

dhruvachak
Copy link
Contributor

With this change, TargetRegionOpId is no more used and hence deleted.

@dhruvachak
[OpenMP] [OMPT] A pointer to HostOpId should be passed in EMI callbacks.
9bd39db 
With this change, TargetRegionOpId is no more used and hence deleted.
@llvmbot llvmbot added the openmp:libomptarget OpenMP offload runtime label Dec 15, 2023
jplehr
jplehr reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this, Dhruva. Do we have a test that covers this?

@dhruvachak
Copy link
Contributor Author

Thanks for taking care of this, Dhruva. Do we have a test that covers this?

Though we have tests for OMPT that exercise this code path, it is not clear how to test for it unless we have support for trace records, which we don't yet in upstream code. Before this patch, the tool is getting a pointer to the wrong location. Unless there is a consumer of the correct location, I am not sure how to have a test to detect this problem.

@dhruvachak
Copy link
Contributor Author

Thanks for taking care of this, Dhruva. Do we have a test that covers this?

Though we have tests for OMPT that exercise this code path, it is not clear how to test for it unless we have support for trace records, which we don't yet in upstream code. Before this patch, the tool is getting a pointer to the wrong location. Unless there is a consumer of the correct location, I am not sure how to have a test to detect this problem.

There is a test downstream (with support for trace records) if that helps. ROCm/aomp#762

@jprotze
Copy link
Collaborator

jprotze commented Dec 15, 2023

Is there something that stops us to upstream these tests?

@dhruvachak
Copy link
Contributor Author

Is there something that stops us to upstream these tests?

The functionality (trace record support) that could catch the problem is not yet upstreamed. So the test would not apply.

jplehr
jplehr approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

Ok, makes sense.
LGTM

@dhruvachak dhruvachak merged commit e4de6a6 into llvm:main Dec 15, 2023
5 checks passed
@dhruvachak dhruvachak deleted the dc_fix_host_op_id_2 branch December 15, 2023 20:07
@dhruvachak dhruvachak mentioned this pull request Feb 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplehr jplehr jplehr approved these changes

@mhalk mhalk Awaiting requested review from mhalk

Assignees
No one assigned
Labels
openmp:libomptarget OpenMP offload runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dhruvachak @jprotze @jplehr @llvmbot