llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Julian Brown (jtb20) <details> <summary>Changes</summary> Per OpenMP 6.0 [7.2], the 'saved' modifier on a data-environment attribute clause has effect only when that clause appears on a replayable construct. Per [14.3] and [14.6], the only directives that admit the 'replayable' clause are: 'target', 'target enter data', 'target exit data', 'target update', 'task', 'taskloop' and 'taskwait'. Of those seven, only 'target', 'task' and 'taskloop' also admit a 'firstprivate' clause; the remaining four cannot syntactically carry 'firstprivate' and need no extra sema work. A directive outside the admitted set can never make 'firstprivate(saved: ...)' meaningful. We do not try to flag every dead-modifier case statically: 'saved' on a directive in the admitted set but without 'replayable' (and without lexical nesting in a taskgraph) is well-formed per [7.2] and silently has no effect. Detecting that would require inspecting the full clause list, and is invalidated as soon as the user adds the clause or moves the construct inside a taskgraph. What we can and do diagnose is the case where the host directive itself cannot ever be a replayable construct: 'saved' on a 'parallel', 'for', 'sections', 'single', 'distribute', 'teams', etc. firstprivate clause has no path to replay semantics at all. Reject that in ActOnOpenMPFirstprivateClause when CurDir is neither an OpenMP tasking directive (isOpenMPTaskingDirective, which covers OMPD_task and every OMPD_*taskloop* variant) nor an OpenMP target execution directive (isOpenMPTargetExecutionDirective, which covers OMPD_target and every combined directive that has OMPD_target as a leaf construct), with a new dedicated diagnostic err_omp_firstprivate_saved_wrong_directive that points at the modifier location and lists the three admissible directives. The existing ast-print test is extended to cover these cases, and other new tests have been added. Assisted-By: Claude Opus 4.7 --- Full diff: https://github.com/llvm/llvm-project/pull/200407.diff 4 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3) - (modified) clang/lib/Sema/SemaOpenMP.cpp (+14) - (modified) clang/test/OpenMP/taskgraph_firstprivate_saved_ast_print.cpp (+45-7) - (added) clang/test/OpenMP/taskgraph_firstprivate_saved_messages.cpp (+115) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index fd442a7b93492..ce6e7f2182d13 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12646,6 +12646,9 @@ def err_omp_one_defaultmap_each_category: Error< def err_omp_lastprivate_conditional_non_scalar : Error< "expected list item of scalar type in 'lastprivate' clause with 'conditional' modifier" >; +def err_omp_firstprivate_saved_wrong_directive : Error< + "'saved' modifier on 'firstprivate' clause is only allowed on a 'task', " + "'taskloop', or 'target' construct">; def err_omp_flush_order_clause_and_list : Error< "'flush' directive with memory order clause '%0' cannot have the list">; def note_omp_flush_order_clause_here : Note< diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 51a5c3522a5db..bf888892f8aab 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -19648,6 +19648,20 @@ OMPClause *SemaOpenMP::ActOnOpenMPFirstprivateClause( return nullptr; } + // OpenMP 6.0 [7.2]: the 'saved' modifier has effect only on a clause + // that appears on a replayable construct. Per [14.6] the directives + // that admit a 'replayable' clause and that also admit 'firstprivate' + // are 'task', 'taskloop', and 'target' (the other replayable-eligible + // directives -- 'target_enter_data' etc. -- do not admit + // 'firstprivate' at all). Reject 'saved' on any other directive, as + // it can never become a replayable construct. + OpenMPDirectiveKind CurDir = DSAStack->getCurrentDirective(); + if (FPKind == OMPC_FIRSTPRIVATE_saved && !isOpenMPTaskingDirective(CurDir) && + !isOpenMPTargetExecutionDirective(CurDir)) { + Diag(FPKindLoc, diag::err_omp_firstprivate_saved_wrong_directive); + return nullptr; + } + SmallVector<Expr *, 8> Vars; SmallVector<Expr *, 8> PrivateCopies; SmallVector<Expr *, 8> Inits; diff --git a/clang/test/OpenMP/taskgraph_firstprivate_saved_ast_print.cpp b/clang/test/OpenMP/taskgraph_firstprivate_saved_ast_print.cpp index faf6b7a2936ae..df358df14c82f 100644 --- a/clang/test/OpenMP/taskgraph_firstprivate_saved_ast_print.cpp +++ b/clang/test/OpenMP/taskgraph_firstprivate_saved_ast_print.cpp @@ -21,21 +21,59 @@ void firstprivate_saved() { #pragma omp task firstprivate(a) { (void)a; } + // 'saved' on an omp task lexically inside a taskgraph. + // CHECK: #pragma omp taskgraph // CHECK: #pragma omp task firstprivate(saved: a) - #pragma omp task firstprivate(saved: a) - { (void)a; } + #pragma omp taskgraph + { + #pragma omp task firstprivate(saved: a) + { (void)a; } + } + // Multiple variables. + // CHECK: #pragma omp taskgraph // CHECK: #pragma omp task firstprivate(saved: a,b,c) - #pragma omp task firstprivate(saved: a, b, c) - { (void)a; (void)b; (void)c; } + #pragma omp taskgraph + { + #pragma omp task firstprivate(saved: a, b, c) + { (void)a; (void)b; (void)c; } + } + // Mixed with another clause. + // CHECK: #pragma omp taskgraph // CHECK: #pragma omp task firstprivate(saved: a) shared(b) - #pragma omp task firstprivate(saved: a) shared(b) - { (void)a; (void)b; } + #pragma omp taskgraph + { + #pragma omp task firstprivate(saved: a) shared(b) + { (void)a; (void)b; } + } + // 'saved' on an omp taskloop lexically inside a taskgraph. + // CHECK: #pragma omp taskgraph // CHECK: #pragma omp taskloop firstprivate(saved: a) - #pragma omp taskloop firstprivate(saved: a) + #pragma omp taskgraph + { + #pragma omp taskloop firstprivate(saved: a) + for (int i = 0; i < 4; ++i) (void)a; + } + + // 'saved' on a replayable omp task outside any taskgraph - also legal. + // CHECK: #pragma omp task replayable firstprivate(saved: a) + #pragma omp task replayable firstprivate(saved: a) + { (void)a; } + + // 'saved' on a replayable omp taskloop outside any taskgraph - also legal. + // CHECK: #pragma omp taskloop replayable firstprivate(saved: a) + #pragma omp taskloop replayable firstprivate(saved: a) for (int i = 0; i < 4; ++i) (void)a; + + // 'saved' on a non-lexically-nested task (dynamic nesting via a call into + // a function from a taskgraph region is the runtime use case) - we accept + // any task/taskloop construct since the static check cannot prove dynamic + // nesting. + // CHECK: #pragma omp task firstprivate(saved: a) + #pragma omp task firstprivate(saved: a) + { (void)a; } } #endif diff --git a/clang/test/OpenMP/taskgraph_firstprivate_saved_messages.cpp b/clang/test/OpenMP/taskgraph_firstprivate_saved_messages.cpp new file mode 100644 index 0000000000000..5197d16cd0421 --- /dev/null +++ b/clang/test/OpenMP/taskgraph_firstprivate_saved_messages.cpp @@ -0,0 +1,115 @@ +// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=60 -fsyntax-only -verify %s + +// Tests the OpenMP 6.0 'saved' modifier on the 'firstprivate' clause. The +// modifier is meaningful only on constructs that create tasks or taskloops +// (the units of work that can participate in taskgraph replay). Every other +// directive that admits a 'firstprivate' clause must reject it. + +void unknown_modifier() { + int a = 0; + // The diagnostic comes from the generic "expected <list> in OpenMP clause" + // path and enumerates the legal modifier names ('saved' in OpenMP 6.0). + #pragma omp task firstprivate(bogus: a) // expected-error {{expected 'saved' in OpenMP clause 'firstprivate'}} + { (void)a; } +} + +void rejected_on_non_tasking_constructs() { + int a = 0; + int b[8]; + + // parallel + #pragma omp parallel firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + { (void)a; } + + // for (worksharing) + #pragma omp parallel + { + #pragma omp for firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + for (int i = 0; i < 4; ++i) (void)a; + } + + // sections + #pragma omp parallel + { + #pragma omp sections firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + { + (void)a; + } + } + + // single + #pragma omp parallel + { + #pragma omp single firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + { (void)a; } + } + + // teams (inside target -- the inner directive is a standalone 'teams', + // not a 'target teams' combined directive, so it is not a target + // execution directive in its own right). + #pragma omp target + #pragma omp teams firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + { (void)a; } + + // distribute (inside teams) -- standalone 'distribute' is not a target + // execution directive either. + #pragma omp target teams + #pragma omp distribute firstprivate(saved: a) // expected-error {{'saved' modifier on 'firstprivate' clause is only allowed on a 'task', 'taskloop', or 'target' construct}} + for (int i = 0; i < 4; ++i) (void)a; +} + +void accepted_on_task_taskloop_and_target() { + int a = 0; + + // Bare task (no enclosing taskgraph, no replayable clause): accepted. + // Per OpenMP 6.0 [7.2] the 'saved' modifier silently has no effect on a + // non-replayable construct; we only enforce the directive-kind check + // statically. In this implementation a bare task / taskloop is never + // recorded into a taskgraph -- recording requires either lexical + // nesting inside a '#pragma omp taskgraph' or an explicit 'replayable' + // clause -- so the modifier is a well-defined no-op here. + #pragma omp task firstprivate(saved: a) + { (void)a; } + + // Bare taskloop: accepted, same rationale. + #pragma omp taskloop firstprivate(saved: a) + for (int i = 0; i < 4; ++i) (void)a; + + // Replayable task: explicitly opted-in for replay. + #pragma omp task replayable firstprivate(saved: a) + { (void)a; } + + // Replayable taskloop. + #pragma omp taskloop replayable firstprivate(saved: a) + for (int i = 0; i < 4; ++i) (void)a; + + // Task lexically nested inside a taskgraph. + #pragma omp taskgraph + { + #pragma omp task firstprivate(saved: a) + { (void)a; } + } + + // Bare target: accepted on the same well-formed-but-no-effect grounds + // as a bare task. Per OpenMP 6.0 [14.6] the 'target' construct admits + // both 'firstprivate' and 'replayable', so 'saved' is meaningful as + // soon as a 'replayable' clause is added or the construct is nested + // inside a 'taskgraph' region. + #pragma omp target firstprivate(saved: a) + { (void)a; } + + // Replayable target: explicitly opted-in for replay. + #pragma omp target replayable firstprivate(saved: a) + { (void)a; } + + // Combined target construct (target + parallel): accepted because the + // composite directive is a target execution directive in its own + // right, so the captured snapshot at the target boundary belongs to a + // construct that may participate in taskgraph replay. + #pragma omp target parallel firstprivate(saved: a) + { (void)a; } + + // Combined target teams distribute parallel for: same rationale. + #pragma omp target teams distribute parallel for firstprivate(saved: a) + for (int i = 0; i < 4; ++i) (void)a; +} `````````` </details> https://github.com/llvm/llvm-project/pull/200407 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
