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

[CIR][OpenMP] Initial commit for OpenMP support in CIR #382

Merged
merged 6 commits into from
Jan 22, 2024
Merged

[CIR][OpenMP] Initial commit for OpenMP support in CIR #382

merged 6 commits into from
Jan 22, 2024

Conversation

fabianmcg
Copy link
Contributor

@fabianmcg fabianmcg commented Jan 4, 2024 •
edited
Loading

This patch introduces initial support for:

pragma omp parallel

This patch doesn't add support for any of the parallel clauses, including variable privatization; thus, all variables are handled as shared.

This PR fixes issue #285.

@bcardosolopes bcardosolopes mentioned this pull request Jan 9, 2024
Open
bcardosolopes
bcardosolopes requested changes Jan 9, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Hi Fabian, this is awesome! Excited to see this PR and upcoming work for OpenMP. Please also add in the description that this fixes #285

Thanks for breaking this into incremental work, way to go! Comments inline, mostly nit.

clang/lib/CIR/CodeGen/CIRGenDecl.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Outdated
if (D) {
if (LangOpts.OpenMP && !LangOpts.OpenMPSimd)
assert(0 && "not implemented");
// TODO[OpenMP]: Check and handle target globals.
Copy link
Member

Choose a reason for hiding this comment

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

This should also be assert(!UnimplementedFeature::openMPRuntime());

clang/lib/CIR/CodeGen/CIRGenModule.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenStmtOpenMP.cpp Show resolved Hide resolved
clang/test/CIR/CodeGen/openmp.cpp Outdated Show resolved Hide resolved
@fabianmcg
Copy link
Contributor Author

Yes, I'll switch al the TODOS to UnimplementedFeature.

However, the reason I left those TODOS, it's because in many cases to properly check the appropriate condition I needed to add more lines of code and I didn't want to bloat the patch. All the assertions I removed were the minimal set so that the added tests worked.

@fabianmcg
[CIR][OpenMP] Initial commit for OpenMP support in CIR
b2f2563 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses, including
variable privatization; thus, all variables are handled as shared.
@fabianmcg
Addressed reviewer comments
b385148 
I left the `TODO[OpenMP]:` comments in `CIRGenExpr.cpp` as it is necessary to remove
assertions to test and ithat specific issue needs to be addressed quickly in a
different patch on captured variables.
bcardosolopes
bcardosolopes requested changes Jan 19, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes 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 the update, almost there! You missed to address some comments (or didn't explain why you took a different approach).

clang/lib/CIR/CodeGen/CIRGenStmtOpenMP.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Show resolved Hide resolved
bcardosolopes
bcardosolopes requested changes Jan 19, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

One last comment remains

clang/lib/CIR/CodeGen/CIRGenDecl.cpp Outdated Show resolved Hide resolved
bcardosolopes
bcardosolopes approved these changes Jan 19, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Sweet! LGTM, will merge as soon as I get green signals

@kiranchandramohan
Copy link
Contributor

This is really exciting @fabianmcg.

@fabianmcg
Copy link
Contributor Author

Thank you @kiranchandramohan!

@bcardosolopes bcardosolopes merged commit ef3f6a5 into llvm:main Jan 22, 2024
6 checks passed
lanza pushed a commit that referenced this pull request Jan 29, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
e03cb18 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
@fabianmcg fabianmcg deleted the omp-dev branch February 7, 2024 19:40
lanza pushed a commit that referenced this pull request Mar 23, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
5768230 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
d129b76 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
6578b6e 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
lanza pushed a commit that referenced this pull request Apr 29, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
5186df1 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
lanza pushed a commit that referenced this pull request Apr 29, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
3392f7d 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
8702aab 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
lanza pushed a commit that referenced this pull request Apr 29, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
c003cb3 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
4a814cc 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
80745b7 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (llvm#382)
549d078 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue llvm#285.
lanza pushed a commit that referenced this pull request Nov 5, 2024
@fabianmcg @lanza
[CIR][OpenMP] Initial commit for OpenMP support in CIR (#382)
8a01bc5 
This patch introduces initial support for:
```
pragma omp parallel
```

This patch doesn't add support for any of the `parallel` clauses,
including variable privatization; thus, all variables are handled as
shared.

This PR fixes issue #285.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardosolopes bcardosolopes bcardosolopes approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@fabianmcg @kiranchandramohan @bcardosolopes