[flang-commits] [flang] [OpenMP]Update use_device_clause lowering (PR #101703)

[Sergio Afonso via flang-commits]

================
@@ -788,35 +829,40 @@ static void genIntermediateCommonBlockAccessors(
 
 // This functions creates a block for the body of the targetOp's region. It adds
 // all the symbols present in mapSymbols as block arguments to this block.
-static void
-genBodyOfTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
-                  semantics::SemanticsContext &semaCtx,
-                  lower::pft::Evaluation &eval, mlir::omp::TargetOp &targetOp,
-                  llvm::ArrayRef<const semantics::Symbol *> mapSyms,
-                  llvm::ArrayRef<mlir::Location> mapSymLocs,
-                  llvm::ArrayRef<mlir::Type> mapSymTypes,
-                  DataSharingProcessor &dsp,
-                  const mlir::Location &currentLocation,
-                  const ConstructQueue &queue, ConstructQueue::iterator item) {
+static void genBodyOfTargetAndDataOp(
----------------
skatrak wrote:

In my opinion, this is not the best way to share map-related functionality between `target` and `target_data`. By making both call a single function and then conditionally doing things depending on who made the call, the function becomes harder to understand and maintain. My comment was in the direction of trying to simplify these `genBodyOfTargetOp` and `genBodyOfTargetDataOp` that had grown complex while having large similarities.

```c++
static void bindMapSymbols(...) {
  auto cloneBounds = [cloneBound](llvm::ArrayRef<mlir::Value> bounds) {
    llvm::SmallVector<mlir::Value> clonedBounds;
    for (mlir::Value bound : bounds)
      clonedBounds.emplace_back(cloneBound(bound));
    return clonedBounds;
  };
  // Bind the symbols to their corresponding block arguments.
  for (auto [argIndex, argSymbol] : llvm::enumerate(mapSyms)) {
    ...
  }
}

template<typename OpTy>
static void cloneBoundsDependencies(OpTy op, ...) {
  static_assert(std::is_same_v<OpTy, mlir::omp::TargetOp> ||
                std::is_same_v<OpTy, mlir::omp::TargetDataOp>,
                "OpTy must be TargetOp or TargetDataOp");
  // Check if cloning the bounds introduced any dependency on the outer region.
  // If so, then either clone them as well if they are MemoryEffectFree, or else
  // copy them to a new temporary and add them to the map and block_argument
  // lists and replace their uses with the new temporary.
  llvm::SetVector<mlir::Value> valuesDefinedAbove;
  mlir::getUsedValuesDefinedAbove(region, valuesDefinedAbove);
  while (!valuesDefinedAbove.empty()) {
    for (mlir::Value val : valuesDefinedAbove) {
      ...
      op.getMapVarsMutable().append(mapOp);
      ...
    }
  }
}

static void genBodyOfTargetDataOp(...) {
  ...
  firOpBuilder.createBlock(&region, {}, useDeviceTypes, useDeviceLocs);
  bindMapSymbols(...);
  cloneBoundsDependencies(dataOp, ...);
  ...
}

static void genBodyOfTargetOp(...) {
  ...
  llvm::SmallVector<mlir::Type> allRegionArgTypes;
  mergePrivateVarsInfo(...);
  ...
  auto *regionBlock = firOpBuilder.createBlock(&region, {}, allRegionArgTypes, allRegionArgLocs);
  bindMapSymbols(...);
  for (auto [argIndex, argSymbol] :
       llvm::enumerate(dsp.getAllSymbolsToPrivatize())) {
    ...
  }
  cloneBoundsDependencies(targetOp, ...);
  ...
}
```
Let me know what you think.

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