[flang-commits] [flang] [flang][OpenMP] Privatize loop IVs in the innermost parallel (PR #227486)

[Tom Eccles via flang-commits]

================
@@ -525,43 +491,55 @@ void DataSharingProcessor::collectSymbolsInNestedRegions(
     llvm::SetVector<const semantics::Symbol *> &symbolsInNestedRegions) {
   if (!eval.hasNestedEvaluations())
     return;
+  const semantics::Scope *curScope = getCurrentScope(semaCtx, eval);
+  if (!curScope)
+    return;
+  llvm::SetVector<const semantics::Symbol *> collectedSymbols;
   for (pft::Evaluation &nestedEval : eval.getNestedEvaluations()) {
     if (isOpenMPPrivatizingEvaluation(nestedEval)) {
-      converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
+      converter.collectSymbolSet(nestedEval, collectedSymbols, flag,
                                  /*collectSymbols=*/true,
                                  /*collectHostAssociatedSymbols=*/false);
     } else {
       // Recursively look for OpenMP constructs within `nestedEval`'s region
-      collectSymbolsInNestedRegions(nestedEval, flag, symbolsInNestedRegions);
+      collectSymbolsInNestedRegions(nestedEval, flag, collectedSymbols);
     }
   }
+  for (const semantics::Symbol *sym : collectedSymbols)
+    if (sym->owner() != *curScope)
+      symbolsInNestedRegions.insert(sym);
 }
 
-// Collect all scopes associated with `eval` and return the current scope.
-static const semantics::Scope *
-collectScopes(semantics::SemanticsContext &semaCtx,
-              lower::pft::Evaluation &eval,
-              llvm::SetVector<const semantics::Scope *> &clauseScopes) {
-  std::function<void(const semantics::Scope *)> collect =
-      [&](const semantics::Scope *scope) {
-        clauseScopes.insert(scope);
-        for (const semantics::Scope &child : scope->children())
-          collect(&child);
-      };
-  parser::CharBlock source = getSource(semaCtx, eval);
-  const semantics::Scope *curScope = nullptr;
-  if (!source.empty()) {
-    curScope = &semaCtx.FindScope(source);
-    collect(curScope);
-  }
-  return curScope;
-}
-
+// Collect symbols that `eval` must privatize, but whose data-sharing attributes
+// (DSA) are not explicitly determined.
+//
+// `flag` selects the kind of symbols being collected:
+//   - OmpPrivate / OmpFirstPrivate: symbols privatized by a DEFAULT clause.
+//   - OmpImplicit: symbols with an implicitly determined DSA.
+//   - OmpPreDetermined: symbols with a predetermined DSA.
+//   - std::nullopt: indirect references (see collectIndirectReferences()).
+//
+// `allSymbols` contains the symbols referenced in `eval` (or the indirect
+// references, when `flag` is not set).
+// `symbolsInNestedRegions` contains the symbols referenced in privatizing
+// constructs nested in `eval` that are not owned by `eval`'s scope. When
+// `flag` is set, both sets are restricted to symbols that have it.
+//
+// Excluding some special cases, a symbol is privatized if it is in
+// `allSymbols` but not in `symbolsInNestedRegions`, and it is owned by
+// `eval`'s scope, meaning semantics assigned its DSA to this construct.
+//
+// The selected symbols are added to `allPrivatizedSymbols`, and also to
+// `*symbols` when `symbols` is not null.
 void DataSharingProcessor::collectPrivatizedSymbols(
     std::optional<semantics::Symbol::Flag> flag,
     const llvm::SetVector<const semantics::Symbol *> &allSymbols,
     const llvm::SetVector<const semantics::Symbol *> &symbolsInNestedRegions,
     llvm::SetVector<const semantics::Symbol *> *symbols) {
+  const semantics::Scope *curScope = getCurrentScope(semaCtx, eval);
+  if (!curScope)
+    return;
----------------
tblah wrote:

Are there legitimate cases where this happens or do you think we should have an assertion too?

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