[OpenMP] OpenMP 5.1 "assume" directive parsing support #92731
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: None (jtb20) ChangesThis is a minimal patch to support parsing for "omp assume" directives. These are meant to be hints to a compiler's optimisers: as such, it is legitimate (if not very useful) to ignore them. The patch builds on top of the existing support for "omp assumes" directives (note spelling!). Unlike the "omp [begin/end] assumes" directives, "omp assume" is associated with a compound statement, i.e. it can appear within a function. The "holds" assumption could (theoretically) be mapped onto the existing builtin "__builtin_assume", though the latter applies to a single point in the program, and the former to a range (i.e. the whole of the associated compound statement). This patch fixes sollve's OpenMP 5.1 "omp assume"-based tests. Full diff: https://github.com/llvm/llvm-project/pull/92731.diff 7 Files Affected:
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index e959dd6378f46..5c1bb58227fde 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2450,6 +2450,7 @@ Parser::DeclGroupPtrTy Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(
case OMPD_target_teams_loop:
case OMPD_parallel_loop:
case OMPD_target_parallel_loop:
+ case OMPD_assume:
Diag(Tok, diag::err_omp_unexpected_directive)
<< 1 << getOpenMPDirectiveName(DKind);
break;
@@ -3035,6 +3036,27 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective(
<< 1 << getOpenMPDirectiveName(DKind);
SkipUntil(tok::annot_pragma_openmp_end);
break;
+ case OMPD_assume: {
+ ParseScope OMPDirectiveScope(this, Scope::FnScope | Scope::DeclScope |
+ Scope::CompoundStmtScope);
+ ParseOpenMPAssumesDirective(DKind, ConsumeToken());
+
+ SkipUntil(tok::annot_pragma_openmp_end);
+
+ ParsingOpenMPDirectiveRAII NormalScope(*this);
+ StmtResult AssociatedStmt;
+ {
+ Sema::CompoundScopeRAII Scope(Actions);
+ AssociatedStmt = ParseStatement();
+ EndLoc = Tok.getLocation();
+ Directive = Actions.ActOnCompoundStmt(Loc, EndLoc,
+ AssociatedStmt.get(),
+ /*isStmtExpr=*/false);
+ }
+ ParseOpenMPEndAssumesDirective(Loc);
+ OMPDirectiveScope.Exit();
+ break;
+ }
case OMPD_unknown:
default:
Diag(Tok, diag::err_omp_unknown_directive);
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 6110e5229b076..9ec2bd9c3b802 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3511,7 +3511,8 @@ void SemaOpenMP::ActOnOpenMPAssumesDirective(SourceLocation Loc,
auto *AA =
OMPAssumeAttr::Create(getASTContext(), llvm::join(Assumptions, ","), Loc);
- if (DKind == llvm::omp::Directive::OMPD_begin_assumes) {
+ if (DKind == llvm::omp::Directive::OMPD_begin_assumes ||
+ DKind == llvm::omp::Directive::OMPD_assume) {
OMPAssumeScoped.push_back(AA);
return;
}
diff --git a/clang/test/OpenMP/assume_lambda.cpp b/clang/test/OpenMP/assume_lambda.cpp
new file mode 100644
index 0000000000000..a38380ed4482a
--- /dev/null
+++ b/clang/test/OpenMP/assume_lambda.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -ast-print %s | FileCheck %s
+// expected-no-diagnostics
+
+extern int bar(int);
+
+int foo(int arg)
+{
+ #pragma omp assume no_openmp_routines
+ {
+ auto fn = [](int x) { return bar(x); };
+// CHECK: auto fn = [](int x) {
+ return fn(5);
+ }
+}
+
+class C {
+public:
+ int foo(int a);
+};
+
+// We're really just checking that this parses. All the assumptions are thrown
+// away immediately for now.
+int C::foo(int a)
+{
+ #pragma omp assume holds(sizeof(T) == 8) absent(parallel)
+ {
+ auto fn = [](int x) { return bar(x); };
+// CHECK: auto fn = [](int x) {
+ return fn(5);
+ }
+}
\ No newline at end of file
diff --git a/clang/test/OpenMP/assume_messages.c b/clang/test/OpenMP/assume_messages.c
new file mode 100644
index 0000000000000..33c1c6f7c51e7
--- /dev/null
+++ b/clang/test/OpenMP/assume_messages.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -verify -fopenmp -x c -std=c99 %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -verify -fopenmp-simd -x c -std=c99 %s
+
+#pragma omp assume no_openmp // expected-error {{unexpected OpenMP directive '#pragma omp assume'}}
+
+void foo(void) {
+ #pragma omp assume hold(1==1) // expected-warning {{valid assume clauses start with 'ext_', 'absent', 'contains', 'holds', 'no_openmp', 'no_openmp_routines', 'no_parallelism'; tokens will be ignored}} expected-note {{the ignored tokens spans until here}}
+ {}
+}
+
+void bar(void) {
+ #pragma omp assume absent(target)
+} // expected-error {{expected statement}}
+
+void qux(void) {
+ #pragma omp assume extra_bits // expected-warning {{valid assume clauses start with 'ext_', 'absent', 'contains', 'holds', 'no_openmp', 'no_openmp_routines', 'no_parallelism'; token will be ignored}}
+ {}
+}
+
+void quux(void) {
+ #pragma omp assume ext_spelled_properly
+ {}
+}
diff --git a/clang/test/OpenMP/assume_messages_attr.c b/clang/test/OpenMP/assume_messages_attr.c
new file mode 100644
index 0000000000000..47504cc6308ea
--- /dev/null
+++ b/clang/test/OpenMP/assume_messages_attr.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -verify -fopenmp -x c -std=c99 %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -verify -fopenmp-simd -x c -std=c99 %s
+
+[[omp::directive(assume no_openmp)]] // expected-error {{unexpected OpenMP directive '#pragma omp assume'}}
+
+void foo(void) {
+ [[omp::directive(assume hold(1==1))]] // expected-warning {{valid assume clauses start with 'ext_', 'absent', 'contains', 'holds', 'no_openmp', 'no_openmp_routines', 'no_parallelism'; tokens will be ignored}} expected-note {{the ignored tokens spans until here}}
+ {}
+}
+
+void bar(void) {
+ [[omp::directive(assume absent(target))]]
+} // expected-error {{expected statement}}
+
+void qux(void) {
+ [[omp::directive(assume extra_bits)]] // expected-warning {{valid assume clauses start with 'ext_', 'absent', 'contains', 'holds', 'no_openmp', 'no_openmp_routines', 'no_parallelism'; token will be ignored}}
+ {}
+}
+
+void quux(void) {
+ [[omp::directive(assume ext_spelled_properly)]]
+ {}
+}
diff --git a/clang/test/OpenMP/assume_template.cpp b/clang/test/OpenMP/assume_template.cpp
new file mode 100644
index 0000000000000..b0591bffb20a6
--- /dev/null
+++ b/clang/test/OpenMP/assume_template.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -verify -fopenmp -ast-print %s | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -std=c++11 -include-pch %t -verify %s -ast-print | FileCheck %s
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+extern int qux(int);
+
+template<typename T>
+int foo(T arg)
+{
+ #pragma omp assume no_openmp_routines
+ {
+ auto fn = [](int x) { return qux(x); };
+// CHECK: auto fn = [](int x) {
+ return fn(5);
+ }
+}
+
+template<typename T>
+class C {
+ T m;
+
+public:
+ T bar(T a);
+};
+
+// We're really just checking this parses. All the assumptions are thrown
+// away immediately for now.
+template<typename T>
+T C<T>::bar(T a)
+{
+ #pragma omp assume holds(sizeof(T) == 8) absent(parallel)
+ {
+ return (T)qux((int)a);
+// CHECK: return (T)qux((int)a);
+ }
+}
+
+#endif
\ No newline at end of file
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index e91169e8da1aa..3660917e3e176 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -2089,6 +2089,9 @@ def OMP_Scan : Directive<"scan"> {
];
let association = AS_Separating;
}
+def OMP_Assume : Directive<"assume"> {
+ let association = AS_Block;
+}
def OMP_Assumes : Directive<"assumes"> {
let association = AS_None;
}
|
|
Adding @jdoerfert as potential reviewer |
jtb20
force-pushed
the
main
branch
2 times, most recently
from
c06ce37 to
49870fb
Compare
llvmbot
added
clang:codegen
|
Ping? |
clang/test/OpenMP/assume_lambda.cpp
Outdated
| // CHECK: auto fn = [](int x) { | ||
| return fn(5); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ugh, a bad habit of that editor. I guess I should try to figure out a way of stopping it doing that automatically... (thanks!)
There was a problem hiding this comment.
I mean, we need an empty line at the end of file.
There was a problem hiding this comment.
Understood! There is indeed a vscode option to add the missing newline (it appears to be turned off by default for some bizarre reason). I'll push a new version with them in.
| } | ||
| } | ||
|
|
||
| #endif |
jtb20
force-pushed
the
main
branch
2 times, most recently
from
822249a to
edbcd82
Compare
Sorry, I don't quite understand the question! Could you elaborate a little please? |
I was thinking maybe you need changes in AST related files, like |
At the moment, since the "assume" directive is parsed but then immediately discarded, I don't think anything else is needed. Actually the existing "assumes" support is reused -- the bit in SemaOpenMP.cpp adds the "assume" assumptions to the OMPAssumeScoped "stack". For "assumes", it's done like that so (e.g. top-level) declarations between begin/end "assumes" can be modified according to the assumptions on that stack. For "assume", once some use is made for the assumptions, that might turn out to not be the most useful representation. That can be revisited later though, I think. |
Even so, it should support serialization/deserialization and have the tests for it (like ast-dump and ast-print). |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The compiler builds for each intermediate step, but the result isn't necessarily useful until the 'omp assume' patch. |
clang/include/clang/AST/StmtOpenMP.h
Outdated
|
|
||
| // It's not really an executable directive, but it seems convenient to use | ||
| // that as the parent class. | ||
| class OMPAssumeDirective : public OMPExecutableDirective { |
There was a problem hiding this comment.
class OMPAssumeDirective final : public OMPExecutableDirective
| @@ -377,6 +377,8 @@ bool checkFailClauseParameter(OpenMPClauseKind FailClauseParameter); | |||
| /// otherwise - false. | |||
| bool isOpenMPExecutableDirective(OpenMPDirectiveKind DKind); | |||
|
|
|||
| bool isOpenMPInformationalDirective(OpenMPDirectiveKind DKind); | |||
There was a problem hiding this comment.
Add a comment for the function
| @@ -3532,6 +3532,10 @@ class Parser : public CodeCompletionHandler { | |||
| OpenMPDirectiveKind DKind, SourceLocation Loc, | |||
| bool ReadDirectiveWithinMetadirective); | |||
|
|
|||
| StmtResult ParseOpenMPInformationalDirective( | |||
There was a problem hiding this comment.
Add a comment for the function
| @@ -399,6 +399,14 @@ class SemaOpenMP : public SemaBase { | |||
| OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName, | |||
| OpenMPDirectiveKind CancelRegion, ArrayRef<OMPClause *> Clauses, | |||
| Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc); | |||
| StmtResult ActOnOpenMPInformationalDirective( | |||
There was a problem hiding this comment.
Add a comment for the function
| OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName, | ||
| ArrayRef<OMPClause *> Clauses, Stmt *AStmt, SourceLocation StartLoc, | ||
| SourceLocation EndLoc); | ||
| StmtResult ActOnOpenMPAssumeDirective(ArrayRef<OMPClause *> Clauses, |
There was a problem hiding this comment.
Add a comment for the function
clang/lib/Sema/TreeTransform.h
Outdated
| for (ArrayRef<OMPClause *>::iterator I = Clauses.begin(), E = Clauses.end(); | ||
| I != E; ++I) { |
There was a problem hiding this comment.
| for (ArrayRef<OMPClause *>::iterator I = Clauses.begin(), E = Clauses.end(); | |
| I != E; ++I) { | |
| for (OMPClause *C : Clauses) { |
clang/lib/Sema/TreeTransform.h
Outdated
| if (D->getDirectiveKind() == OMPD_assume) | ||
| CS = D->getAssociatedStmt(); | ||
| else | ||
| llvm_unreachable("Unexpected informational directive"); |
There was a problem hiding this comment.
| if (D->getDirectiveKind() == OMPD_assume) | |
| CS = D->getAssociatedStmt(); | |
| else | |
| llvm_unreachable("Unexpected informational directive"); | |
| assert(D->getDirectiveKind() == OMPD_assume && "Unexpected informational directive"); | |
| CS = D->getAssociatedStmt(); |
clang/lib/Sema/TreeTransform.h
Outdated
| if (AssociatedStmt.isInvalid()) { | ||
| return StmtError(); | ||
| } |
clang/lib/Sema/TreeTransform.h
Outdated
| if (TClauses.size() != Clauses.size()) { | ||
| return StmtError(); | ||
| } |
This patch supports parsing of "omp assume" directives. These are meant to be hints to a compiler's optimisers: as such, it is legitimate (if not very useful) to ignore them. This version of the patch implements new AST nodes for the "assume" directive and the various clauses it accepts as arguments, and adds new (C++ module) tests for serialization/deserialization of said nodes. The patch now uses tail allocation for the directive kind list. Unlike the "omp [begin/end] assumes" directives, "omp assume" is associated with a compound statement, i.e. it can appear within a function. The "holds" assumption could (theoretically) be mapped onto the existing builtin "__builtin_assume", though the latter applies to a single point in the program, and the former to a range (i.e. the whole of the associated compound statement). The "assume" directive appears to be distinct from the [[omp::assume]] annotation. This patch fixes sollve's OpenMP 5.1 "omp assume"-based tests.
|
Thank you for the review! Again, I do not have commit access, so please apply for me if possible. Julian |
|
@jtb20 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
This broke the Flang build. You need to add some Flang code for all changes in omp.td. Look for a similar clause in the flang subproject to see what to do, and verify by building Flang. Maybe revert for now until fixed. |
|
Hopefully fixed by: #102008 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/4246 Here is the relevant piece of the build log for the reference: |
|
This is still breaking a buildbot. https://lab.llvm.org/buildbot/#/builders/157/builds/4246 Are you working on a fix? |
|
On 06/08/2024 19:40, Valentin Clement (バレンタイン クレメン) wrote:
This is still breaking a buildbot.
https://lab.llvm.org/buildbot/#/builders/157/builds/4246
<https://lab.llvm.org/buildbot/#/builders/157/builds/4246>
Are you working on a fix?
It seems to be the same bug -- I assumed the buildbot picked up a
pre-fixed revision. Is that not the case?
Thanks,
Julian
|
|
Yeah it was probably the case. It's green now. |
|
Thank you for the update! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/5113 Here is the relevant piece of the build log for the reference: |
| @@ -510,6 +528,18 @@ def OMP_EndAssumes : Directive<"end assumes"> { | |||
| let association = AS_Delimited; | |||
| let category = OMP_Assumes.category; | |||
| } | |||
| def OMP_Assume : Directive<"assume"> { | |||
| let association = AS_Block; | |||
There was a problem hiding this comment.
I don't understand the question. Do you think the .td fragment is unnecessary for adding support for the "omp assume" directive? There is no comment here, but that follows the general trend in that file.
This is a minimal patch to support parsing for "omp assume" directives. These are meant to be hints to a compiler's optimisers: as such, it is legitimate (if not very useful) to ignore them. The patch builds on top of the existing support for "omp assumes" directives (note spelling!).
Unlike the "omp [begin/end] assumes" directives, "omp assume" is associated with a compound statement, i.e. it can appear within a function. The "holds" assumption could (theoretically) be mapped onto the existing builtin "__builtin_assume", though the latter applies to a single point in the program, and the former to a range (i.e. the whole of the associated compound statement).
This patch fixes sollve's OpenMP 5.1 "omp assume"-based tests.