[Flang] [OpenMP] Add semantic checks for detach clause in task #119172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-semantics Author: Thirumalai Shaktivel (Thirumalai-Shaktivel) ChangesFixes:
Restrictions:
OpenMP 5.2: Detach contruct
Full diff: https://github.com/llvm/llvm-project/pull/119172.diff 4 Files Affected:
|
Thirumalai-Shaktivel
added
flang
You can test this locally with the following command:View the diff from clang-format here. |
|
There seems to be some problem with I get the following locally The failed line seems to be within the range (80 characters): |
Fixes: - Add semantic checks along with the tests - Move the detach clause to allowedOnceClauses list in Task construct Restrictions:\ OpenMP 5.0: Task construct - At most one detach clause can appear on the directive. - If a detach clause appears on the directive, then a mergeable clause cannot appear on the same directive. OpenMP 5.2: Detach contruct - If a detach clause appears on a directive, then the encountering task must not be a final task. - A variable that appears in a detach clause cannot appear as a list item on a data-environment attribute clause on the same construct. - A variable that is part of another variable (as an array element or a structure element) cannot appear in a detach clause. - event-handle must not have the POINTER attribute.
Thirumalai-Shaktivel
force-pushed
the
llvm/detach_01
branch
from
3da984c to
414ea0c
Compare
Thirumalai-Shaktivel
changed the title
kparzysz
reviewed
Dec 10, 2024
| } | ||
| auto type{name->symbol->GetType()}; | ||
| if (!name->symbol->GetType()->IsNumeric(TypeCategory::Integer) || | ||
| evaluate::ToInt64(type->numericTypeSpec().kind()) != 8) { |
There was a problem hiding this comment.
Please remove the check of the kind. We shouldn't be using hardcoded numbers.
There was a problem hiding this comment.
Yes, I felt the same. But, the only info I had was the kind => 8 to throw an error for the following:
!ERROR: The event-handle: `e` must be of type integer(kind=omp_event_handle_kind)
!$omp task detach(e)
x = x + 1
!$omp end task
Do you know any other way to add a check for this?
There was a problem hiding this comment.
We don't have a way of checking this, unfortunately. In other cases we limit it to checking that it has an INTEGER type, but without checking the kind.
| @@ -2739,6 +2739,59 @@ void OmpStructureChecker::Leave(const parser::OmpClauseList &) { | |||
| llvm::omp::Clause::OMPC_copyprivate, {llvm::omp::Clause::OMPC_nowait}); | |||
| } | |||
|
|
|||
| if (GetContext().directive == llvm::omp::Directive::OMPD_task) { | |||
| if (auto *d_clause{FindClause(llvm::omp::Clause::OMPC_detach)}) { | |||
There was a problem hiding this comment.
No snake_case please (d_clause).
There was a problem hiding this comment.
Good catch, I forgot to update this.
Comment on lines
+2744
to
+2748
| // OpenMP 5.0: Task construct restrictions | ||
| CheckNotAllowedIfClause( | ||
| llvm::omp::Clause::OMPC_detach, {llvm::omp::Clause::OMPC_mergeable}); | ||
|
|
||
| // OpenMP 5.2: Task construct restrictions |
There was a problem hiding this comment.
If you're doing version-specific checks, please guard them with the actual version from -fopenmp-version. You can get it from the language options in context. This is done in a number of places in this file, you can check how to do it.
Comment on lines
+2758
to
+2772
| std::string eventHandleSymName{name->ToString()}; | ||
| auto checkVarAppearsInDataEnvClause = [&](const parser::OmpObjectList | ||
| &objs, | ||
| std::string clause) { | ||
| for (const auto &obj : objs.v) { | ||
| if (const parser::Name *objName{ | ||
| parser::Unwrap<parser::Name>(obj)}) { | ||
| if (objName->ToString() == eventHandleSymName) { | ||
| context_.Say(GetContext().clauseSource, | ||
| "A variable: `%s` that appears in a DETACH clause cannot appear on %s clause on the same construct"_err_en_US, | ||
| eventHandleSymName, clause); | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Please compare symbols, not names. You can compare the results of &symbol->GetUltimate() to be accurate.
Comment on lines
+3772
to
+3775
| // OpenMP 5.0: Task construct restrictions | ||
| CheckAllowedClause(llvm::omp::Clause::OMPC_detach); | ||
|
|
||
| // OpenMP 5.2: Detach clause restrictions |
There was a problem hiding this comment.
Same here: please do version-specific checks based on the actual version.
|
Sure, thanks for the review. I will update it and report back soon. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes:
Restrictions:
OpenMP 5.0: Task construct
OpenMP 5.2: Detach contruct