[flang-commits] [flang] [llvm] [Flang][OpenMP] Prevent unrequired implicit maps for private variables on composite directives (PR #217121)

[via flang-commits]

================
@@ -4053,6 +4080,254 @@ static void collectSymbolsWithDynamicSubstring(
   symbolsWithDynamicSubstring = visitor.symbolsWithDynamicSubstring;
 }
 
+// Reverse search a composite/combined directive and return the innermost leaf
+// carrying a private clause. The OpenMP specification dictates that the first
+// leaf that can legally have a private clause becomes the owner, except when an
+// OpenMP 6.0+ directive-name-modifier explicitly attaches clauses to another
+// leaf or construct in the compound directive.
+//
+// TODO: Extend handling to support directive-name-modifier usage placing
+// private clauses on multiple leaf nodes or constituent constructs.
+static ConstructQueue::const_iterator
+findInnermostPrivateLeaf(ConstructQueue::const_iterator begin,
+                         const ConstructQueue &queue) {
+  auto hasPrivate = [](ConstructQueue::const_iterator it) {
+    return llvm::any_of(it->clauses, [](const Clause &clause) {
+      return std::holds_alternative<clause::Private>(clause.u);
+    });
+  };
+  for (auto it = queue.end(); it != begin;) {
+    --it;
+    if (hasPrivate(it))
+      return it;
+  }
+  return queue.end();
+}
+
+static List<Clause>
+getOpenMPConstructClauses(const parser::OpenMPConstruct &ompEval,
+                          semantics::SemanticsContext &semaCtx) {
+  List<Clause> clauses;
+  const parser::OmpClauseList *beginClauseList{nullptr};
+  const parser::OmpClauseList *endClauseList{nullptr};
+
+  common::visit(
+      [&](const auto &construct) {
+        using Type = llvm::remove_cvref_t<decltype(construct)>;
+        if constexpr (std::is_same_v<Type, parser::OmpBlockConstruct> ||
+                      std::is_same_v<Type, parser::OpenMPLoopConstruct>) {
+          beginClauseList = &construct.BeginDir().Clauses();
+          if (auto &endSpec{construct.EndDir()})
+            endClauseList = &endSpec->Clauses();
+        }
+      },
+      ompEval.u);
+
+  if (!beginClauseList)
+    return clauses;
+
+  clauses.append(makeClauses(*beginClauseList, semaCtx));
+
+  if (endClauseList)
+    clauses.append(makeClauses(*endClauseList, semaCtx));
+
+  return clauses;
+}
+
+static bool isSymbolUsedInClauseRange(const semantics::Symbol &target,
+                                      ConstructQueue::const_iterator begin,
+                                      ConstructQueue::const_iterator end,
+                                      bool ignorePrivateAndMap) {
+  const semantics::Symbol &ultimateTarget = target.GetUltimate();
+
+  for (ConstructQueue::const_iterator it = begin; it != end; ++it) {
+    for (const Clause &clause : it->clauses) {
+      // Do not consider private itself. Also do not consider map clauses here:
+      // explicit target-level maps are already accounted for via mapObjects,
+      // and construct decomposition may synthesize map-like clauses for nested
+      // private list items.
+      if (ignorePrivateAndMap &&
+          (clause.id == llvm::omp::Clause::OMPC_private ||
+           clause.id == llvm::omp::Clause::OMPC_map))
+        continue;
+
+      if (visitSymbolsInClause(clause, [&](const semantics::Symbol &sym) {
+            return sym.GetUltimate() == ultimateTarget;
+          }))
+        return true;
+    }
+  }
+  return false;
+}
+
+// Verify if it is safe to drop the map and materialize a target-local clone.
+// Certain cases still require a map to be OpenMP specification compliant:
+//
+//  Allocatable -
+//    1) If the status is unallocated, the private allocatable is unallocated
+//    2) If the allocation status is allocated, the private allocatable is
+//       allocated
+//    3) If it is an array, the allocatable's shape and bounds are preserved.
+//  Pointer - The initial status of a private pointer is undefined.
+//  Other - If the type of the list item has default initialization, the new
+//    list item has default initialization. Otherwise, it is undefined.
+//
+// From the above list, it is currently fine to drop maps and materialize clones
+// for scalars, pointers, and constant arrays. Their creation does not require
+// dynamic extent/shape/allocation-status values defined outside the target
+// region. Types that still need a map just to populate the initial state are
+// allocatables, derived types with allocatable or pointer ultimate components,
+// assumed-shape arrays, automatic arrays, and common block members. Currently
+// we leave these as they were before, but in the future we can minimize the
+// mapping by sending across exactly what is required to be specification
+// compliant.
+static bool isTargetLocalCloneable(const semantics::Symbol &sym,
----------------
agozillon wrote:

Not sure I fully understand the request, but I've modified the name and parameter in an attempt to be a bit more explanative, the comment and internal code should hopefully fill in the remaining gaps.

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