-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[OpenMP 5.2] New syntax for 'uses_allocators' clause #157025
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.
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
Conversation
@llvm/pr-subscribers-clang Author: Urvi Rav (ravurvi20) ChangesThis patch updates the parsing changes to handle the new syntax of the
Full diff: https://github.com/llvm/llvm-project/pull/157025.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index bc7a6e231d93c..44c3d07bf6027 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1497,6 +1497,11 @@ def err_omp_multiple_step_or_linear_modifier : Error<
"multiple %select{'step size'|'linear modifier'}0 found in linear clause">;
def err_omp_deprecate_old_syntax: Error<
"old syntax '%0' on '%1' clause was deprecated, use new syntax '%2'">;
+def warn_omp_deprecate_old_syntax: Warning<
+ "old syntax '%0' on '%1' clause was deprecated, use new syntax '%2'">,
+ InGroup<Deprecated>;
+def err_omp_allocator_comma_separator :
+ Error<"',' not allowed as separator in 'uses_allocators' clause, use ';' instead">;
def warn_omp_future_directive_spelling: Warning<
"directive spelling '%0' is introduced in a later OpenMP version">,
InGroup<OpenMPFuture>;
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index 5db2f2e2ccf86..92abc7084bed0 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2959,6 +2959,69 @@ OMPClause *Parser::ParseOpenMPUsesAllocatorClause(OpenMPDirectiveKind DKind) {
return nullptr;
SmallVector<SemaOpenMP::UsesAllocatorsData, 4> Data;
do {
+ // Parse 'traits(expr) : Allocator' for >=5.2
+ if (getLangOpts().OpenMP >= 52 &&
+ Tok.is(tok::identifier) &&
+ Tok.getIdentifierInfo()->getName() == "traits") {
+
+ SemaOpenMP::UsesAllocatorsData &D = Data.emplace_back();
+
+ ConsumeToken();
+
+ // Parse '(' <expr> ')'
+ BalancedDelimiterTracker TraitParens(*this, tok::l_paren, tok::annot_pragma_openmp_end);
+ TraitParens.consumeOpen();
+ ExprResult AllocatorTraits =
+ getLangOpts().CPlusPlus ? ParseCXXIdExpression() : ParseExpression();
+ TraitParens.consumeClose();
+
+ if (AllocatorTraits.isInvalid()) {
+ SkipUntil({tok::comma, tok::semi, tok::r_paren, tok::annot_pragma_openmp_end},
+ StopBeforeMatch);
+ break;
+ }
+
+ // Expect ':'
+ if (Tok.isNot(tok::colon)) {
+ Diag(Tok, diag::err_expected) << tok::colon;
+ SkipUntil({tok::comma, tok::semi, tok::r_paren, tok::annot_pragma_openmp_end},
+ StopBeforeMatch);
+ continue;
+ }
+ ConsumeToken();
+
+ CXXScopeSpec SS;
+ Token Replacement;
+ ExprResult AllocatorExpr =
+ getLangOpts().CPlusPlus
+ ? ParseCXXIdExpression()
+ : tryParseCXXIdExpression(SS, /*isAddressOfOperand=*/false, Replacement);
+
+ if (AllocatorExpr.isInvalid()) {
+ SkipUntil({tok::comma, tok::semi, tok::r_paren, tok::annot_pragma_openmp_end},
+ StopBeforeMatch);
+ break;
+ }
+
+ D.Allocator = AllocatorExpr.get();
+ D.AllocatorTraits = AllocatorTraits.get();
+ D.LParenLoc = TraitParens.getOpenLocation();
+ D.RParenLoc = TraitParens.getCloseLocation();
+
+ // Separator handling(;)
+ if (Tok.is(tok::comma)) {
+ // In 5.2, comma is invalid
+ Diag(Tok.getLocation(), diag::err_omp_allocator_comma_separator)
+ << FixItHint::CreateReplacement(Tok.getLocation(), ";");
+ ConsumeAnyToken();
+ } else if (Tok.is(tok::semi)) {
+ ConsumeAnyToken(); // valid separator
+ }
+
+ continue;
+ }
+
+ // Parse 'Allocator(expr)' for <5.2
CXXScopeSpec SS;
Token Replacement;
ExprResult Allocator =
@@ -2988,6 +3051,14 @@ OMPClause *Parser::ParseOpenMPUsesAllocatorClause(OpenMPDirectiveKind DKind) {
D.AllocatorTraits = AllocatorTraits.get();
D.LParenLoc = T.getOpenLocation();
D.RParenLoc = T.getCloseLocation();
+
+ // Deprecation diagnostic in >= 5.2
+ if (getLangOpts().OpenMP >= 52) {
+ Diag(Loc, diag::warn_omp_deprecate_old_syntax)
+ << "allocator(expr)" // %0: old form
+ << "uses_allocators" // %1: clause name
+ << "traits(expr): alloc"; // %2: suggested new form
+ }
}
if (Tok.isNot(tok::comma) && Tok.isNot(tok::r_paren))
Diag(Tok, diag::err_omp_expected_punc) << "uses_allocators" << 0;
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index a1dd5b090c59b..7db5226da4cb5 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -24083,11 +24083,13 @@ OMPClause *SemaOpenMP::ActOnOpenMPUsesAllocatorClause(
// OpenMP [2.12.5, target Construct]
// Non-predefined allocators appearing in a uses_allocators clause must
// have traits specified.
+ if (getLangOpts().OpenMP < 52) {
if (!IsPredefinedAllocator && !D.AllocatorTraits) {
Diag(D.Allocator->getExprLoc(),
diag::err_omp_nonpredefined_allocator_without_traits);
continue;
}
+ }
// No allocator traits - just convert it to rvalue.
if (!D.AllocatorTraits)
AllocatorExpr = SemaRef.DefaultLvalueConversion(AllocatorExpr).get();
diff --git a/clang/test/OpenMP/target_uses_allocators_messages.cpp b/clang/test/OpenMP/target_uses_allocators_messages.cpp
index b145c4d9501cf..45906bc92999e 100644
--- a/clang/test/OpenMP/target_uses_allocators_messages.cpp
+++ b/clang/test/OpenMP/target_uses_allocators_messages.cpp
@@ -2,6 +2,10 @@
// RUN: %clang_cc1 -verify -fopenmp-simd %s -Wuninitialized
+// RUN: %clang_cc1 -DOMP52 -verify=omp52 -fopenmp -fopenmp-version=52 %s -Wuninitialized
+
+// RUN: %clang_cc1 -DOMP52 -verify=omp52 -fopenmp-simd -fopenmp-version=52 %s -Wuninitialized
+
struct omp_alloctrait_t {};
typedef void **omp_allocator_handle_t;
@@ -16,10 +20,15 @@ extern const omp_allocator_handle_t omp_pteam_mem_alloc;
extern const omp_allocator_handle_t omp_thread_mem_alloc;
int main(int argc, char **argv) {
- omp_alloctrait_t traits[10];
+ omp_alloctrait_t my_traits[10];
omp_alloctrait_t *ptraits;
omp_allocator_handle_t my_alloc = nullptr;
const omp_allocator_handle_t c_my_alloc = my_alloc;
+ omp_alloctrait_t aligned_traits[10];
+ omp_allocator_handle_t aligned_alloc;
+
+#ifndef OMP52
+
#pragma omp target uses_allocators // expected-error {{expected '(' after 'uses_allocator'}}
{}
#pragma omp target uses_allocators( // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{expected unqualified-id}}
@@ -34,7 +43,7 @@ int main(int argc, char **argv) {
{}
#pragma omp target uses_allocators(omp_default_mem_alloc, omp_large_cap_mem_alloc, omp_const_mem_alloc, omp_high_bw_mem_alloc, omp_low_lat_mem_alloc, omp_cgroup_mem_alloc, omp_pteam_mem_alloc, omp_thread_mem_alloc)
{}
-#pragma omp target uses_allocators(omp_default_mem_alloc(traits), omp_large_cap_mem_alloc(traits), omp_const_mem_alloc(traits), omp_high_bw_mem_alloc(traits), omp_low_lat_mem_alloc(traits), omp_cgroup_mem_alloc(traits), omp_pteam_mem_alloc(traits), omp_thread_mem_alloc(traits)) // expected-error 8 {{predefined allocator cannot have traits specified}} expected-note-re 8 {{predefined trait '{{omp_default_mem_alloc|omp_large_cap_mem_alloc|omp_const_mem_alloc|omp_high_bw_mem_alloc|omp_low_lat_mem_alloc|omp_cgroup_mem_alloc|omp_pteam_mem_alloc|omp_thread_mem_alloc}}' used here}}
+#pragma omp target uses_allocators(omp_default_mem_alloc(my_traits), omp_large_cap_mem_alloc(my_traits), omp_const_mem_alloc(my_traits), omp_high_bw_mem_alloc(my_traits), omp_low_lat_mem_alloc(my_traits), omp_cgroup_mem_alloc(my_traits), omp_pteam_mem_alloc(my_traits), omp_thread_mem_alloc(my_traits)) // expected-error 8 {{predefined allocator cannot have traits specified}} expected-note-re 8 {{predefined trait '{{omp_default_mem_alloc|omp_large_cap_mem_alloc|omp_const_mem_alloc|omp_high_bw_mem_alloc|omp_low_lat_mem_alloc|omp_cgroup_mem_alloc|omp_pteam_mem_alloc|omp_thread_mem_alloc}}' used here}}
{}
#pragma omp target uses_allocators(my_alloc, c_my_alloc) // expected-error {{non-predefined allocator must have traits specified}} expected-error {{expected variable of the 'omp_allocator_handle_t' type, not 'const omp_allocator_handle_t' (aka 'void **const')}}
{}
@@ -46,10 +55,20 @@ int main(int argc, char **argv) {
{}
#pragma omp target uses_allocators(my_alloc(ptraits)) // expected-error {{expected constant sized array of 'omp_alloctrait_t' elements, not 'omp_alloctrait_t *'}}
{}
-#pragma omp target uses_allocators(my_alloc(traits)) private(my_alloc) // expected-error {{allocators used in 'uses_allocators' clause cannot appear in other data-sharing or data-mapping attribute clauses}} expected-note {{defined as private}}
+#pragma omp target uses_allocators(my_alloc(my_traits)) private(my_alloc) // expected-error {{allocators used in 'uses_allocators' clause cannot appear in other data-sharing or data-mapping attribute clauses}} expected-note {{defined as private}}
+{}
+#pragma omp target map(my_alloc, my_traits) uses_allocators(my_alloc(my_traits)) // expected-error {{allocators used in 'uses_allocators' clause cannot appear in other data-sharing or data-mapping attribute clauses}} expected-note {{used here}}
+{}
+#else
+#pragma omp target teams uses_allocators(traits(my_traits): my_alloc; traits(aligned_traits): aligned_alloc)
+{}
+#pragma omp target teams uses_allocators(my_alloc)
+{}
+#pragma omp target teams uses_allocators(traits(my_traits): my_alloc, traits(aligned_traits): aligned_alloc) // omp52-error {{',' not allowed as separator in 'uses_allocators' clause, use ';' instead}}
{}
-#pragma omp target map(my_alloc, traits) uses_allocators(my_alloc(traits)) // expected-error {{allocators used in 'uses_allocators' clause cannot appear in other data-sharing or data-mapping attribute clauses}} expected-note {{used here}}
+#pragma omp target teams uses_allocators(my_alloc(my_traits), aligned_alloc(aligned_traits)) // omp52-warning {{old syntax 'allocator(expr)' on 'uses_allocators' clause was deprecated, use new syntax 'traits(expr): alloc'}} //omp52-warning {{old syntax 'allocator(expr)' on 'uses_allocators' clause was deprecated, use new syntax 'traits(expr): alloc'}}
{}
+#endif
return 0;
}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
b975a4b
to
cfe1df3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update OpenMP.rst and whatsnew docs for clang
def warn_omp_deprecate_old_syntax: Warning< | ||
"old syntax '%0' on '%1' clause was deprecated, use new syntax '%2'">, | ||
InGroup<Deprecated>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want a warning instead of error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexey-bataev Since the construct is deprecated and not removed, I kept it as a warning for backward compatibility. I can change it to an error if preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is: is the old syntax still allowed in 6.0? If not, it should be the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looked into the spec (Section B.2, Version 5.2 to 6.0 Differences) which states: All features deprecated in versions 5.0, 5.1 and 5.2 were removed.
Since the old syntax falls under this, it is no longer allowed in 6.0. Based on this, I’ve updated it to emit an error instead of a warning.
04875d1
to
51f5f07
Compare
ping @alexey-bataev |
def warn_omp_deprecate_old_syntax: Warning< | ||
"old syntax '%0' on '%1' clause was deprecated, use new syntax '%2'">, | ||
InGroup<Deprecated>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is: is the old syntax still allowed in 6.0? If not, it should be the error.
def err_omp_allocator_comma_separator : | ||
Error<"',' not allowed as separator in 'uses_allocators' clause, use ';' instead">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to reuse existing message? I think there should be something similar already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked through the file to see if there’s an existing message we could reuse, but didn’t find anything relevant (using ;
instead of ,
) If that’s fine, can we move ahead with this as is?
Also update clang release notes document |
done!! |
| device | clause: extended device | :good:`done` | | | ||
+------------------------------+--------------------------------------------------------------+--------------------------+-----------------------------------------------------------------------+ | ||
| device | clause: uses_allocators clause | :good:`done` | | | ||
| device | clause: uses_allocators clause | :good:`done` | https://github.com/llvm/llvm-project/pull/157025 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do anything in codegen? If yes, this should be partial
, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a parsing change for the new uses_allocator
syntax, no codegen required; works the same way, so keeping it as done
.
@ravurvi20 merging this PR, since already approved !! |
This patch updates the parsing changes to handle the new syntax of the
uses_allocators
clause as defined in OpenMP 5.2(Section 6.8).