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

Added OpenMP 5.0 specification based lowering of atomic read and write #1367

Open
wants to merge 1 commit into
base: fir-dev
Choose a base branch
from
Open

Added OpenMP 5.0 specification based lowering of atomic read and write #1367

wants to merge 1 commit into from

Conversation

NimishMishra
Copy link

This PR adds PFT to MLIR lowering support for atomic read and write.

PeixinQiao
PeixinQiao reviewed Mar 15, 2022
View reviewed changes
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/test/Lower/OpenMP/atomic01.f90 Show resolved Hide resolved
@kiranchandramohan
Copy link
Collaborator

@NimishMishra Will you be able to address the changes requested by Eric and Peixin?

@NimishMishra
Copy link
Author

NimishMishra commented Mar 21, 2022 •
edited
Loading

@kiranchandramohan I got caught up elsewhere. I'll pick this PR on priority. I see Eric is fine with ReferenceType. I don't have any idea on HeapType, PointerType, and LLVMPointerType. Can you give some idea on which one to pick up?

@NimishMishra NimishMishra force-pushed the atomic-lowering branch 4 times, most recently from 065fd89 to 5bf06d4 Compare March 25, 2022 06:20
@NimishMishra
Copy link
Author

@kiranchandramohan Can we merge this?

PeixinQiao
PeixinQiao reviewed Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@PeixinQiao PeixinQiao left a comment

Choose a reason for hiding this comment

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

Can you give the high priority for this PR? I would like to use PointerLikeModel in Threadprivate codegen.

flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
@PeixinQiao
Copy link
Collaborator

Except for those code style issues, the function code looks good to me.

BTW, it seems that some semantic checks are missing for atomic construct. For example, at most one hint clause can be in atomic construct, and which memory order clauses can be in which atomic construct. Not sure if they are beyond OpenMP 1.1.

@NimishMishra
Copy link
Author

Except for those code style issues, the function code looks good to me.

BTW, it seems that some semantic checks are missing for atomic construct. For example, at most one hint clause can be in atomic construct, and which memory order clauses can be in which atomic construct. Not sure if they are beyond OpenMP 1.1.

I'll do the code style changes. As far as the semantic checks are concerned, I have verified that they are present in llvm main repository. I did not implement them; they were already there.

I'll take this PR on priority.

PeixinQiao
PeixinQiao approved these changes Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@PeixinQiao PeixinQiao left a comment

Choose a reason for hiding this comment

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

Well done. Thanks @NimishMishra .

flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
kiranchandramohan
kiranchandramohan reviewed Mar 27, 2022
View reviewed changes
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

Could you create a copy of this for upstream llvm-project as well?

I have a couple of points. I can merge this if other reviewers are OK with the patch.

flang/lib/Optimizer/Dialect/FIRType.cpp
: public mlir::omp::PointerLikeType::ExternalModel<
AlternativePointerLikeModel<T>, T> {
mlir::Type getElementType(mlir::Type pointer) const {
return pointer.cast<T>().getEleTy();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once upstreaming is complete we should check with @schweitzpgi whether it is OK for all these types to have the same getElementType function.

Copy link
Author

Choose a reason for hiding this comment

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

@kiranchandramohan There's a little hunch with the upstreaming. I will ask on slack. Meanwhile, I addressed all the nits and pushed an update here. Upstreaming may take a while I guess

flang/lib/Optimizer/Dialect/FIRType.cpp Outdated
Comment on lines 954 to 961
fir::PointerType::attachInterface<PointerLikeModel<fir::PointerType>>(
*getContext());

fir::HeapType::attachInterface<AlternativePointerLikeModel<fir::PointerType>>(
*getContext());

fir::LLVMPointerType::attachInterface<
AlternativePointerLikeModel<fir::PointerType>>(*getContext());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do all these take in a fir::PointerType?

Copy link
Author

Choose a reason for hiding this comment

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

No. It was an error from my end. I have fixed them now.

flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
@NimishMishra
Copy link
Author

Could you create a copy of this for upstream llvm-project as well?

Yes. I will create a upstream differential for this PR.

@kiranchandramohan
Copy link
Collaborator

@shraiysh have you reviewed this PR? If you are OK, please approve.

shraiysh
shraiysh approved these changes Mar 29, 2022
View reviewed changes
Copy link
Collaborator

@shraiysh shraiysh left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP.cpp Outdated Show resolved Hide resolved
kiranchandramohan
kiranchandramohan approved these changes Apr 1, 2022
View reviewed changes
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LGTM.

@NimishMishra
Copy link
Author

Note: This PR can no longer be merged since #1570 is merged. Once we reach a consensus on https://reviews.llvm.org/D122725, I will modify this PR too to that end.

@kiranchandramohan
Copy link
Collaborator

Can you close this now and cherry-pick D122725 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeixinQiao PeixinQiao PeixinQiao approved these changes

@shraiysh shraiysh shraiysh approved these changes

@kiranchandramohan kiranchandramohan kiranchandramohan approved these changes

@schweitzpgi schweitzpgi Awaiting requested review from schweitzpgi

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

Successfully merging this pull request may close these issues.
5 participants
@NimishMishra @kiranchandramohan @PeixinQiao @schweitzpgi @shraiysh