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

[Leandro Lupori via flang-commits]

================
@@ -3952,6 +3967,147 @@ static void collectSymbolsWithDynamicSubstring(
   symbolsWithDynamicSubstring = visitor.symbolsWithDynamicSubstring;
 }
 
+// Reverse search the composite/combined directive and return the first leaf
+// carrying a private clause, the OpenMP specification dictates that the first
+// leaf that can legally have a private clause becomes the owner.
+//
+// The function intentionally skips the node representing the root target
+// directive by setting the end (begin) point to the second leaf, the private
+// clauses on a standalone target region should apply through regular
+// privatization means. Otherwise, if we find no relevant leaf node (carrying a
+// private clause) we return the end of the iterator.
+static ConstructQueue::const_iterator
+findInnermostPrivateLeaf(ConstructQueue::const_iterator item,
+                         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);
+    });
+  };
+  auto begin = std::next(item);
+  for (auto it = queue.end(); it != begin;) {
+    --it;
+    if (hasPrivate(it))
+      return it;
+  }
+  return queue.end();
+}
+
+// Verify if it's safe to drop the map and materialize a clone. Certain cases
+// still currently require a map to be OpenMP specification compliant, a
+// rewording of the specfications privatization rules made to be a bit more
+// concise, if the type is an:
+//
+//  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's an array, the allocatables shape and bounds will be
+//    the same.
+//  Pointer - The intial 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's undefined.
+//
+// From the above list, it is currently fine to drop maps and materialize clones
+// for scalars, pointers, and constant arrays. As their creation does not
+// neccesitate referencing dynamic extent/shape/allocation-status values defined
+// outside the target region. Types that fall into this category and that need a
+// map still (just to populate the initial state) are allocatables (and by
+// extension this unfortunately precludes derived types with allocatables in
+// them for now), and assumed shape arrays (size being illegal on a private
+// clause). Currently we leave these as they were before, but in the future we
+// can minimize the mapping by sending across exactly what we require to be
+// specification compliant.
+static bool isTargetLocalCloneable(const semantics::Symbol &sym,
+                                   semantics::SemanticsContext &semaCtx) {
+  const semantics::Symbol &ult = sym.GetUltimate();
+  // Allocatable descriptors cannot be cloned and have their implicit map
+  // blocked, the clone requires the host descriptor for allocation
+  // status/bounds.
+  if (semantics::IsAllocatable(ult))
+    return false;
+
+  // A derived type with an allocatable ultimate component cannot be cloned and
+  // have its implicit map dropped for the same reasons we currently have to
+  // do it for the allocatable case.
+  if (const semantics::DeclTypeSpec *declType = ult.GetType())
+    if (const semantics::DerivedTypeSpec *derived = declType->AsDerived())
+      if (semantics::FindAllocatableUltimateComponent(*derived))
+        return false;
+
+  // Pointers initial status is undefined, so we can just materialize it with
+  // the correct typing, but we need to be a little more careful with anything
+  // else that isn't a scalar and verify that it has constant extents and shape.
+  if (ult.Rank() > 0 && !semantics::IsPointer(ult)) {
+    evaluate::FoldingContext &foldingContext = semaCtx.foldingContext();
+    std::optional<evaluate::Shape> shape =
+        evaluate::GetShape(foldingContext, ult);
+    std::optional<evaluate::ConstantSubscripts> extents =
+        evaluate::AsConstantExtents(foldingContext, shape);
+    if (!extents || evaluate::HasNegativeExtent(*extents))
+      return false;
+  }
+  return true;
+}
+
+// Discover the `private` list items of the inner leaf construct of a
+// combined/composite clause e.g. the "s" in `target teams distribute
+// private(a)`. These are not privatized on the target itself, and without
----------------
luporl wrote:

```suggestion
// combined/composite clause e.g. the "s" in `target teams distribute
// private(s)`. These are not privatized on the target itself, and without
```

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