[llvm-branch-commits] [Clang][OpenMP] Fix capture of reduction variables under nested default(firstprivate) (PR #216329)

[via llvm-branch-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Julian Brown (jtb20)

<details>
<summary>Changes</summary>

isOpenMPCapturedByRef() decided whether a scalar is captured by copy for a
given capture Level, but consulted the default data-sharing attribute of the
innermost directive (getDefaultDSA()) instead of the one at that Level
(getDefaultDSA(Level)).

When a construct with default(firstprivate) (or default(private)) is nested in
an enclosing captured region within the same function -- e.g. a taskloop with a
reduction inside a 'parallel' region -- this made the enclosing region capture
the reduction variable by copy.  The reduction then combined into that copy and
its result never propagated back to the original variable (it read as 0).

Query the default attribute at the capture Level being considered, matching the
Level already used by the neighbouring hasExplicitDSA()/isLoopControlVariable()
checks.  For top-level captures Level is the top of stack, so behaviour is
unchanged there.

This bug is independent of the 'taskgraph' directive; add a CodeGen regression
test (taskloop_reduction_default_firstprivate_codegen.cpp) exercising it with a
taskloop reduction under default(firstprivate) nested in a plain parallel/single
region.

Assisted-By: Claude Opus 4.8


---
Full diff: https://github.com/llvm/llvm-project/pull/216329.diff


2 Files Affected:

- (modified) clang/lib/Sema/SemaOpenMP.cpp (+7-3) 
- (added) clang/test/OpenMP/taskloop_reduction_default_firstprivate_codegen.cpp (+44) 


``````````diff
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index f0e4788a602be..3c809117e25a6 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -2354,9 +2354,13 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
         !(isa<OMPCapturedExprDecl>(D) && !D->hasAttr<OMPCaptureNoInitAttr>() &&
           !cast<OMPCapturedExprDecl>(D)->getInit()->isGLValue()) &&
         // If the variable is implicitly firstprivate and scalar - capture by
-        // copy
-        !((DSAStack->getDefaultDSA() == DSA_firstprivate ||
-           DSAStack->getDefaultDSA() == DSA_private) &&
+        // copy.  Query the default data-sharing attribute at the capture level
+        // being considered rather than at the innermost directive: a nested
+        // construct's default(firstprivate)/default(private) must not force an
+        // enclosing captured region (e.g. 'taskgraph') to capture a reduction
+        // variable by copy, which would sever the reduction's write-back.
+        !((DSAStack->getDefaultDSA(Level) == DSA_firstprivate ||
+           DSAStack->getDefaultDSA(Level) == DSA_private) &&
           !DSAStack->hasExplicitDSA(
               D, [](OpenMPClauseKind K, bool) { return K != OMPC_unknown; },
               Level) &&
diff --git a/clang/test/OpenMP/taskloop_reduction_default_firstprivate_codegen.cpp b/clang/test/OpenMP/taskloop_reduction_default_firstprivate_codegen.cpp
new file mode 100644
index 0000000000000..0bbd7593c337f
--- /dev/null
+++ b/clang/test/OpenMP/taskloop_reduction_default_firstprivate_codegen.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=60 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=60 -x c++ -std=c++11 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=60 -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// expected-no-diagnostics
+
+// A taskloop with default(firstprivate) and a reduction, nested inside an
+// enclosing captured region (here 'parallel'/'single', with no 'taskgraph'
+// involved).  The reduction variable 'res' must be captured *by reference* by
+// the enclosing parallel region (its address forwarded to the outlined
+// function) so the reduction result propagates back to it.
+//
+// This is the non-taskgraph form of the same bug fixed for taskgraph: when
+// deciding the enclosing region's capture kind, Sema consulted the innermost
+// directive's default(firstprivate) instead of the default at the capture
+// level, capturing 'res' by copy and severing the reduction write-back (the
+// caller then read the unmodified original, i.e. 0).
+
+#ifndef HEADER
+#define HEADER
+
+int run(int seed) {
+  int x = seed;
+  int res = 0;
+
+#pragma omp parallel
+#pragma omp single
+  {
+#pragma omp taskloop num_tasks(4) default(firstprivate) reduction(+ : res)
+    for (int i = 0; i < 8; ++i)
+      res += x + i;
+  }
+
+  return res;
+}
+
+#endif
+
+// CHECK-LABEL: define {{.*}}@_Z3runi(
+// The reduction variable is captured BY REFERENCE by the enclosing parallel
+// region: its address (not its value) is forwarded to the outlined function.
+// CHECK:         call {{.*}}@__kmpc_fork_call(ptr {{.*}}, i32 2, ptr @_Z3runi.omp_outlined, ptr %[[RES:[0-9a-z._]+]], ptr %{{[0-9a-z._]+}})
+// The caller reads the (reduced) value straight back out of that same storage.
+// CHECK:         %[[RET:.*]] = load i32, ptr %[[RES]], align 4
+// CHECK-NEXT:    ret i32 %[[RET]]

``````````

</details>


https://github.com/llvm/llvm-project/pull/216329