[OpenACC] Implement 'break' and 'continue' errors for Compute Cnstrcts #82543
Conversation
OpenACC3.3 2.5.4 says: "A program may not branch into or out of a compute construct". While some of this restriction isn't particularly checkable, 'break' and 'continue' are possible and pretty trivial, so this patch implements those limitations. It IS unclear in the case of a 'break' in a 'switch' what should happen (an antagonistic reading of the standard would prevent it from appearing), however we're choosing to special-case the break-in-switch to ensure that this works (albeit, a 'parallel' directive on a 'switch' isn't particularly useful, though permitted). Future implementations of this rule will be in a follow-up patch.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Erich Keane (erichkeane) ChangesOpenACC3.3 2.5.4 says: "A program may not branch into or out of a compute construct". While some of this restriction isn't particularly checkable, 'break' and 'continue' are possible and pretty trivial, so this patch implements those limitations. It IS unclear in the case of a 'break' in a 'switch' what should happen (an antagonistic reading of the standard would prevent it from Future implementations of this rule will be in a follow-up patch. Full diff: https://github.com/llvm/llvm-project/pull/82543.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 11411883e1bfc6..dbe6eecaa73df4 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12201,4 +12201,6 @@ def warn_acc_clause_unimplemented
def err_acc_construct_appertainment
: Error<"OpenACC construct '%0' cannot be used here; it can only "
"be used in a statement context">;
+def err_acc_branch_in_out
+ : Error<"invalid branch %select{out of|into}0 OpenACC region">;
} // end of sema component.
diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h
index 9e81706cd2aa1d..3ffcc3590ae0e0 100644
--- a/clang/include/clang/Sema/Scope.h
+++ b/clang/include/clang/Sema/Scope.h
@@ -150,6 +150,9 @@ class Scope {
/// template scope in between), the outer scope does not increase the
/// depth of recursion.
LambdaScope = 0x8000000,
+ /// This is the scope of an OpenACC Compute Construct, which restricts
+ /// jumping into/out of it.
+ OpenACCComputeConstructScope = 0x10000000,
};
private:
@@ -469,6 +472,12 @@ class Scope {
return false;
}
+ /// Return true if this exact scope (and not one of it's parents) is a switch
+ /// scope.
+ bool isDirectlySwitchScope() const {
+ return getFlags() & Scope::SwitchScope;
+ }
+
/// Determines whether this scope is the OpenMP directive scope
bool isOpenMPDirectiveScope() const {
return (getFlags() & Scope::OpenMPDirectiveScope);
@@ -504,6 +513,12 @@ class Scope {
return getFlags() & Scope::OpenMPOrderClauseScope;
}
+ /// Determine whether this scope is the statement associated with an OpenACC
+ /// Compute construct directive.
+ bool isOpenACCComputeConstructScope() const {
+ return getFlags() & Scope::OpenACCComputeConstructScope;
+ }
+
/// Determine whether this scope is a while/do/for statement, which can have
/// continue statements embedded into it.
bool isContinueScope() const {
diff --git a/clang/lib/Parse/ParseOpenACC.cpp b/clang/lib/Parse/ParseOpenACC.cpp
index 50e78e8687aea1..4946a61fca007f 100644
--- a/clang/lib/Parse/ParseOpenACC.cpp
+++ b/clang/lib/Parse/ParseOpenACC.cpp
@@ -560,6 +560,21 @@ bool doesDirectiveHaveAssociatedStmt(OpenACCDirectiveKind DirKind) {
llvm_unreachable("Unhandled directive->assoc stmt");
}
+unsigned getOpenACCScopeFlags(OpenACCDirectiveKind DirKind) {
+ switch (DirKind) {
+ case OpenACCDirectiveKind::Parallel:
+ // Mark this as a BreakScope/ContinueScope as well as a compute construct
+ // so that we can diagnose trying to 'break'/'continue' inside of one.
+ return Scope::BreakScope | Scope::ContinueScope |
+ Scope::OpenACCComputeConstructScope;
+ case OpenACCDirectiveKind::Invalid:
+ llvm_unreachable("Shouldn't be creating a scope for an invalid construct");
+ default:
+ break;
+ }
+ return 0;
+}
+
} // namespace
// OpenACC 3.3, section 1.7:
@@ -1228,6 +1243,8 @@ StmtResult Parser::ParseOpenACCDirectiveStmt() {
if (doesDirectiveHaveAssociatedStmt(DirInfo.DirKind)) {
ParsingOpenACCDirectiveRAII DirScope(*this, /*Value=*/false);
+ ParseScope ACCScope(this, getOpenACCScopeFlags(DirInfo.DirKind));
+
AssocStmt = getActions().ActOnOpenACCAssociatedStmt(DirInfo.DirKind,
ParseStatement());
}
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index dde3bd84e89f8b..b1025c05e5db50 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3356,6 +3356,14 @@ Sema::ActOnContinueStmt(SourceLocation ContinueLoc, Scope *CurScope) {
// initialization of that variable.
return StmtError(Diag(ContinueLoc, diag::err_continue_from_cond_var_init));
}
+
+ // A 'continue' that would normally have execution continue on a block outside
+ // of a compute construct counts as 'branching out of' the compute construct,
+ // so diagnose here.
+ if (S->isOpenACCComputeConstructScope())
+ return StmtError(Diag(ContinueLoc, diag::err_acc_branch_in_out)
+ << /*out of */ 0);
+
CheckJumpOutOfSEHFinally(*this, ContinueLoc, *S);
return new (Context) ContinueStmt(ContinueLoc);
@@ -3371,6 +3379,20 @@ Sema::ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope) {
if (S->isOpenMPLoopScope())
return StmtError(Diag(BreakLoc, diag::err_omp_loop_cannot_use_stmt)
<< "break");
+
+ // OpenACC doesn't allow 'break'ing from a compute construct, so diagnose if
+ // we are trying to do so. This can come in 2 flavors: 1-the break'able thing
+ // (besides the compute construct) 'contains' the compute construct, at which
+ // point the 'break' scope will be the compute construct. Else it could be a
+ // loop of some sort that has a direct parent of the compute construct.
+ // However, a 'break' in a 'switch' marked as a compute construct doesn't
+ // count as 'branch out of' the compute construct.
+ if (S->isOpenACCComputeConstructScope() ||
+ (!S->isDirectlySwitchScope() && S->getParent() &&
+ S->getParent()->isOpenACCComputeConstructScope()))
+ return StmtError(Diag(BreakLoc, diag::err_acc_branch_in_out)
+ << /*out of */ 0);
+
CheckJumpOutOfSEHFinally(*this, BreakLoc, *S);
return new (Context) BreakStmt(BreakLoc);
diff --git a/clang/test/SemaOpenACC/no-branch-in-out.c b/clang/test/SemaOpenACC/no-branch-in-out.c
new file mode 100644
index 00000000000000..622cf55f484739
--- /dev/null
+++ b/clang/test/SemaOpenACC/no-branch-in-out.c
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 %s -verify -fopenacc
+
+void BreakContinue() {
+
+#pragma acc parallel
+ for(int i =0; i < 5; ++i) {
+ switch(i) {
+ case 0:
+ break; // leaves switch, not 'for'.
+ default:
+ i +=2;
+ break;
+ }
+ if (i == 2)
+ continue;
+
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ }
+
+ int j;
+ switch(j) {
+ case 0:
+#pragma acc parallel
+ {
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ }
+ case 1:
+#pragma acc parallel
+ {
+ }
+ break;
+ }
+
+#pragma acc parallel
+ for(int i = 0; i < 5; ++i) {
+ if (i > 1)
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ }
+
+#pragma acc parallel
+ switch(j) {
+ case 1:
+ break;
+ }
+
+#pragma acc parallel
+ {
+ for(int i = 1; i < 100; i++) {
+ if (i > 4)
+ break;
+ }
+ }
+
+ for (int i =0; i < 5; ++i) {
+#pragma acc parallel
+ {
+ continue; // expected-error{{invalid branch out of OpenACC region}}
+ }
+ }
+
+#pragma acc parallel
+ for (int i =0; i < 5; ++i) {
+ continue;
+ }
+
+#pragma acc parallel
+ for (int i =0; i < 5; ++i) {
+ {
+ continue;
+ }
+ }
+
+ for (int i =0; i < 5; ++i) {
+#pragma acc parallel
+ {
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ }
+ }
+
+#pragma acc parallel
+ while (j) {
+ --j;
+ if (j > 4)
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ }
+
+#pragma acc parallel
+ do {
+ --j;
+ if (j > 4)
+ break; // expected-error{{invalid branch out of OpenACC region}}
+ } while (j );
+
+}
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/Sema/SemaStmt.cpp
Outdated
| // However, a 'break' in a 'switch' marked as a compute construct doesn't | ||
| // count as 'branch out of' the compute construct. | ||
| if (S->isOpenACCComputeConstructScope() || | ||
| (!S->isDirectlySwitchScope() && S->getParent() && |
There was a problem hiding this comment.
Why do you need to check for non-switch scope here? Is it by default, that if it is openacc block, it is not switch scope? Because you create openacc scope explicitly without setting switch scope, no?
There was a problem hiding this comment.
This is covering 2 separate cases.
First:
switch(F) { <-- Switch scope
case 1:
#pragma acc parallel <-- compute-construct scope
break;
}
In this case, this is the pre-'||' check, the pragma's scope itself (created in ParseOpenACC.cpp) will be the 'break parent' of current, which cannot be broken from (so it is diagnosed). This can NEVER be a 'switch' scope, as it is a 'pragma acc compute-construct' scope.
Second:
#pragma acc parallel <-- compute construct scope
switch(F) { <-- switch scope
case 1:
break;
}
This is how the OpenMP variant of this diagnostic works, the 'scope' here is the 'switch scope'. Additionally, the 'parent' of this one is the Compute Construct scope. So everything after the '||' handles this. (note that isOpenMPLoopScope handles this by implicitly checking the 'parent'). Since we have the 'switch' scope (or other loop/scope/etc), we have to check that it is directly inside of the pragma scope. Since this one CAN be a 'switch'/any other loop/scope/etc, we have to check it, as well as checking the 'parent' for being an OpenACC Compute construct.
There was a problem hiding this comment.
You just need to check that this is a loop scope with the parent openacc scope. break should not be allowed in loops only, right? Why do you check for not-switches? Can you check instead for the loop scope explicitly? Much easier to read and maintain.
There was a problem hiding this comment.
OpenACC compute constructs are NOT limited to loops unfortunately, they can be any statement, so just checking the loop isn't sufficient. And checking the 'is loop with a parent openacc scope' doesn't work if the openacc-pragma is inside the loop. See my 1st example above, the pragma is the 'immediate' scope, but the 'switch' is above it (or the loop in that case).
There was a problem hiding this comment.
The first (dangling) case should be handled by getBreakParent() function. OpenMP set FnScope (IIRC) to nullify break parent and diagnose dangling break/continue and other similar constructs.
There was a problem hiding this comment.
It is not neccessary. I just want to understand why you can check !S->isDirectlySwitchScope() but you cannot check S->isDirectlyLoopScope() instead? What's the difference?
There was a problem hiding this comment.
Sorry if I'm being dumb here, but I don't really get the question? We're trying to avoid diagnosing when the breakParent is a switch, so we are excluding it. If we were to NOT use the inversion, we would be diagnosing ONLY on a the switch (the opposite of what I want?).
So the logic for when to diagnose is:
S->isOpenACCComputeConstructScope() <-- Diagnose if the 'current' breakParent is a Compute Construct
OR
(!S->isDirectlySwitchScope() && S->getParent() && S->getParent()->isOpenACCComputeConstructScope())) <-- If the Parent of the breakParent is a Compute Construct, unless it is a switch.
There was a problem hiding this comment.
No problem we're in the same position. :)
I'm not asking about the very first check, it is ok and I did not ask about it.
What I'm asking is exactly the second part (after OR, second part of the logic). I'm suggesting instead of checking for non-switch just to check explicitly for 'for scope'. "Not switch scope" is too broad, I just suggest to check for the scopes, which are not allowed instead (isForScope). I think it is much easier to read and understand the context.
There was a problem hiding this comment.
No problem we're in the same position. :) I'm not asking about the very first check, it is ok and I did not ask about it. What I'm asking is exactly the second part (after OR, second part of the logic). I'm suggesting instead of checking for non-switch just to check explicitly for 'for scope'. "Not switch scope" is too broad, I just suggest to check for the scopes, which are not allowed instead (isForScope). I think it is much easier to read and understand the context.
Ah! So we don't actually HAVE a ForScope as far as I can tell, let alone the while or do-while: https://clang.llvm.org/doxygen/classclang_1_1Scope.html
Perhaps I could change the name of isDirectlySwitchScope to be: isBreakLoop or something, and have it do the inverse of what isDirectlySwitchScope does now?
There was a problem hiding this comment.
Perhaps I could change the name of isDirectlySwitchScope to be: isBreakLoop or something, and have it do the inverse of what isDirectlySwitchScope does now?
Yep, this will be much better!
The dumpImpl needs a manually written list of elements, so ensure we have the entry so debugging ->dump calls on the scope are emitted properly and don't assert.
OpenACC3.3 2.5.4 says: "A program may not branch into or out of a compute construct". While some of this restriction isn't particularly checkable, 'break' and 'continue' are possible and pretty trivial, so this patch implements those limitations.
It IS unclear in the case of a 'break' in a 'switch' what should happen (an antagonistic reading of the standard would prevent it from
appearing), however we're choosing to special-case the break-in-switch
to ensure that this works (albeit, a 'parallel' directive on a 'switch' isn't particularly useful, though permitted).
Future implementations of this rule will be in a follow-up patch.