[flang][OpenMP] incorrect handling for local variable in OpenMP parallel workshare firstprivate(P)

Project: llvm/llvm-project | Language: C++ | Concepts: concurrency, memory-management | Difficulty: mid
Link: https://github.com/llvm/llvm-project/pull/195616

The problem

When using OpenMP's firstprivate(P) clause in a parallel workshare region in Fortran, where P is an array pointer or allocatable, Flang did not correctly handle the thread-local copy and initialization of P. This could lead to incorrect memory accesses or even segmentation faults, especially when the pointer descriptor is privatized but its target data is shared.

Why it's non-trivial

A naive fix might just make a thread-local copy of the pointer variable. However, in Fortran, a pointer's descriptor (its metadata) can be privatized, but the data it points to may be shared among threads. If initialization steps (like setting the pointer to null) are incorrectly duplicated in each thread, or if stores through the pointer are parallelized when they should not be, this can corrupt program state or cause crashes. Correct handling requires distinguishing between the pointer descriptor and its target, and ensuring initialization and assignment are correctly sequenced and scoped.

The approach

The PR makes several targeted changes to ensure correct handling:

+  // With firstprivate(P) where P is a pointer, each thread gets its own copy
+  // of the descriptor, but P(i) accesses shared target data.
+  if (source.accessPath.hasPointerDeref())
+    return false;

In isOpenMPThreadLocalMemory, this snippet ensures that even if a pointer descriptor is thread-local, a store through the pointer (i.e., to its target) is not considered thread-local. This prevents incorrect parallelization of accesses to shared data.

+    // "firstprivate" pointer initialization creates: (1) alloca, (2) store
+    // null box, (3) copy original. If step (2) is duplicated into the
+    // parallel block, it runs after initialization of the private copy and
+    // overwrites the pointer descriptor with null, causing a segfault on
+    // dereference.
+    SmallPtrSet<Value, 4> hoistedCopyprivateAllocas;

This comment and the associated set track allocations that are used for copyprivate/firstprivate variables. It prevents problematic stores (like zero-initialization of the pointer descriptor) from being duplicated into the parallel region, which could otherwise overwrite the correct value.

+        // Check if this operation writes to a hoisted copyprivate alloca.
+        // Such stores must stay only in the single block; the copyprivate
+        // mechanism handles broadcasting the final value to all threads.
+        bool writesToCopyprivateAlloca = false;
+        if (!hoistedCopyprivateAllocas.empty()) {
+          if (auto memEffects = dyn_cast<MemoryEffectOpInterface>(&op)) {
+            SmallVector<MemoryEffects::EffectInstance> effects;
+            memEffects.getEffects(effects);
+            writesToCopyprivateAlloca =
+                llvm::any_of(effects, [&](const auto &eff) {
+                  return isa<MemoryEffects::Write>(eff.getEffect()) &&
+                         eff.getValue() &&
+                         hoistedCopyprivateAllocas.contains(eff.getValue());
+                });
+          }
+        }
+        if (!writesToCopyprivateAlloca &&
+            llvm::all_of(op.getOperands(), [&](Value opr) {
+              // Either we have already remapped it
+              bool remapped = rootMapping.contains(opr);
+              // Or it is available because it dominates `sr`
+              return remapped || domInfo.dominates(opr.getDefiningOp(), &*sr.begin);
+            })) {
+          // Safe to clone into parallel region
+          singleBuilder.clone(op, singleMapping);
+        }

This logic ensures that stores to privatized descriptors (like the pointer's metadata) are not moved into the parallel region if they are part of the copyprivate/firstprivate initialization sequence. This prevents overwriting the correct value set during privatization.

+        rootMapping.map(&*alloca, &*hoisted);
+        rootMapping.map(alloca.getResult(), hoisted.getResult());
+        copyPrivate.push_back(hoisted);
+        hoistedCopyprivateAllocas.insert(alloca.getResult());

Tracks which allocas are used for privatized variables, so that later code can recognize and treat them specially.

From the review thread

No notable review discussion on this PR.

Takeaways

• When privatizing pointer variables in parallel regions, always distinguish between the pointer descriptor (thread-local) and the data it points to (potentially shared).
• Initialization and assignment steps for privatized variables must be carefully sequenced and scoped to avoid overwriting correct values or introducing race conditions.
• Memory effect analysis is crucial for safely parallelizing code: operations that write to shared or specially-handled memory (like privatized descriptors) must be recognized and handled accordingly.