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

[Sergio Afonso via flang-commits]

================
@@ -180,28 +193,58 @@ void cloneOrMapRegionOutsiders(
       // which comes with a fairly large overhead comparatively. We could be
       // more robust about this and check using a BackwardsSlice to see if we
       // run the risk of mapping a box.
-      if (valOp && mlir::isMemoryEffectFree(valOp) &&
-          !mlir::isa<fir::BoxDimsOp>(valOp)) {
+      bool clonable = valOp && mlir::isMemoryEffectFree(valOp) &&
+          !mlir::isa<fir::BoxDimsOp>(valOp);
+
+      if (clonable) {
         mlir::Operation *clonedOp = valOp->clone();
         entryBlock->push_front(clonedOp);
 
-        auto replace = [entryBlock](mlir::OpOperand &use) {
-          return use.getOwner()->getBlock() == entryBlock;
-        };
-
-        valOp->getResults().replaceUsesWithIf(clonedOp->getResults(), replace);
-        valOp->replaceUsesWithIf(clonedOp, replace);
-      } else {
+        valOp->getResults().replaceUsesWithIf(clonedOp->getResults(), inScope);
+        valOp->replaceUsesWithIf(clonedOp, inScope);
+      } else if (mapNonClonable) {
         mlir::Value mappedTemp = mapTemporaryValue(firOpBuilder, targetOp, val,
             /*name=*/{});
-        val.replaceUsesWithIf(mappedTemp, [entryBlock](mlir::OpOperand &use) {
-          return use.getOwner()->getBlock() == entryBlock;
-        });
+        val.replaceUsesWithIf(mappedTemp, inScope);
       }
     }
+
+    if (!iterateToFixpoint)
+      break;
+
     valuesDefinedAbove.clear();
     mlir::getUsedValuesDefinedAbove(region, valuesDefinedAbove);
-  }
+  } while (!valuesDefinedAbove.empty());
+}
+} // namespace
+
+void cloneOrMapRegionOutsiders(
+    fir::FirOpBuilder &firOpBuilder, mlir::omp::TargetOp targetOp) {
+  mlir::Block *entryBlock = &targetOp.getRegion().getBlocks().front();
+
+  // Iterate to a fixpoint, mapping any non-clonable outsiders. Uses are only
+  // rewired within the entry block itself.
+  resolveRegionOutsiders(firOpBuilder, targetOp, /*mapNonClonable=*/true,
----------------
skatrak wrote:

The `mapNonClonable` and `iterateToFixpoint` flags seem to be set/unset in sync. Would it be a supported case for them to diverge? If not, we should consolidate them into a single one to avoid later misuses of the function and generally make it easier to follow/update its implementation.

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