[Flang][OpenMP] Addressed review comments

Author: kaviya2510

flang/lib/Lower/OpenMP/ClauseProcessor.h

@@ -107,7 +107,7 @@ class ClauseProcessor {
       llvm::SmallVectorImpl<const semantics::Symbol *> &isDeviceSyms) const;
   bool processInReduction(
       mlir::Location currentLocation, mlir::omp::InReductionClauseOps &result,
-      llvm::SmallVectorImpl<const semantics::Symbol *> &InReductionSyms) const;
+      llvm::SmallVectorImpl<const semantics::Symbol *> &outReductionSyms) const;
   bool
   processLink(llvm::SmallVectorImpl<DeclareTargetCapturePair> &result) const;
 
@@ -126,10 +126,9 @@ class ClauseProcessor {
   bool processReduction(
       mlir::Location currentLocation, mlir::omp::ReductionClauseOps &result,
       llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSyms) const;
-  bool processTaskReduction(mlir::Location currentLocation,
-                            mlir::omp::TaskReductionClauseOps &result,
-                            llvm::SmallVectorImpl<const semantics::Symbol *>
-                                &TaskReductionSyms) const;
+  bool processTaskReduction(
+      mlir::Location currentLocation, mlir::omp::TaskReductionClauseOps &result,
+      llvm::SmallVectorImpl<const semantics::Symbol *> &outReductionSyms) const;
   bool processTo(llvm::SmallVectorImpl<DeclareTargetCapturePair> &result) const;
   bool processUseDeviceAddr(
       lower::StatementContext &stmtCtx,

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

@@ -344,11 +344,13 @@ void DataSharingProcessor::collectSymbols(
   // Collect all symbols referenced in the evaluation being processed,
   // that matches 'flag'.
   llvm::SetVector<const semantics::Symbol *> allSymbols;
-  bool collectSymbols = true;
-  for (const omp::Clause &clause : clauses) {
-    if (clause.id == llvm::omp::Clause::OMPC_in_reduction)
-      collectSymbols = false;
-  }
+
+  auto itr = llvm::find_if(clauses, [](const omp::Clause &clause) {
+    return clause.id == llvm::omp::Clause::OMPC_in_reduction;
+  });
+
+  bool collectSymbols = (itr == clauses.end());
+
   converter.collectSymbolSet(eval, allSymbols, flag,
                              /*collectSymbols=*/collectSymbols,
                              /*collectHostAssociatedSymbols=*/true);

flang/lib/Lower/OpenMP/OpenMP.cpp

@@ -1253,21 +1253,20 @@ static void genTaskClauses(
     lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
     lower::StatementContext &stmtCtx, const List<Clause> &clauses,
     mlir::Location loc, mlir::omp::TaskOperands &clauseOps,
-    llvm::SmallVectorImpl<const semantics::Symbol *> &InReductionSyms) {
+    llvm::SmallVectorImpl<const semantics::Symbol *> &inReductionSyms) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
   cp.processDepend(clauseOps);
   cp.processFinal(stmtCtx, clauseOps);
   cp.processIf(llvm::omp::Directive::OMPD_task, clauseOps);
-  cp.processInReduction(loc, clauseOps, InReductionSyms);
+  cp.processInReduction(loc, clauseOps, inReductionSyms);
   cp.processMergeable(clauseOps);
   cp.processPriority(stmtCtx, clauseOps);
   cp.processUntied(clauseOps);
   cp.processDetach(clauseOps);
   // TODO Support delayed privatization.
 
-  cp.processTODO<clause::Affinity>(
-      loc, llvm::omp::Directive::OMPD_task);
+  cp.processTODO<clause::Affinity>(loc, llvm::omp::Directive::OMPD_task);
 }
 
 static void genTaskgroupClauses(
@@ -1888,9 +1887,9 @@ genTaskOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
           ConstructQueue::const_iterator item) {
   lower::StatementContext stmtCtx;
   mlir::omp::TaskOperands clauseOps;
-  llvm::SmallVector<const semantics::Symbol *> InReductionSyms;
+  llvm::SmallVector<const semantics::Symbol *> inReductionSyms;
   genTaskClauses(converter, semaCtx, stmtCtx, item->clauses, loc, clauseOps,
-                 InReductionSyms);
+                 inReductionSyms);
 
   if (!enableDelayedPrivatization)
     return genOpWithBody<mlir::omp::TaskOp>(
@@ -1907,7 +1906,7 @@ genTaskOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
   EntryBlockArgs taskArgs;
   taskArgs.priv.syms = dsp.getDelayedPrivSymbols();
   taskArgs.priv.vars = clauseOps.privateVars;
-  taskArgs.inReduction.syms = InReductionSyms;
+  taskArgs.inReduction.syms = inReductionSyms;
   taskArgs.inReduction.vars = clauseOps.inReductionVars;
 
   auto genRegionEntryCB = [&](mlir::Operation *op) {
@@ -1927,14 +1926,6 @@ genTaskOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
 
   auto taskOp =
       genOpWithBody<mlir::omp::TaskOp>(genInfo, queue, item, clauseOps);
-
-  llvm::SmallVector<mlir::Type> inReductionTypes;
-  for (const auto &inreductionVar : clauseOps.inReductionVars)
-    inReductionTypes.push_back(inreductionVar.getType());
-
-  // Add reduction variables as entry block arguments to the task region
-  llvm::SmallVector<mlir::Location> blockArgLocs(InReductionSyms.size(), loc);
-  taskOp->getRegion(0).addArguments(inReductionTypes, blockArgLocs);
   return taskOp;
 }
 
@@ -1949,21 +1940,23 @@ genTaskgroupOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
   genTaskgroupClauses(converter, semaCtx, item->clauses, loc, clauseOps,
                       taskReductionSyms);
 
+  EntryBlockArgs taskgroupArgs;
+  taskgroupArgs.taskReduction.syms = taskReductionSyms;
+  taskgroupArgs.taskReduction.vars = clauseOps.taskReductionVars;
+
+  auto genRegionEntryCB = [&](mlir::Operation *op) {
+    genEntryBlock(converter.getFirOpBuilder(), taskgroupArgs, op->getRegion(0));
+    return llvm::to_vector(taskgroupArgs.getSyms());
+  };
+
   OpWithBodyGenInfo genInfo =
       OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
                         llvm::omp::Directive::OMPD_taskgroup)
-          .setClauses(&item->clauses);
+          .setClauses(&item->clauses)
+          .setGenRegionEntryCb(genRegionEntryCB);
 
   auto taskgroupOp =
       genOpWithBody<mlir::omp::TaskgroupOp>(genInfo, queue, item, clauseOps);
-
-  llvm::SmallVector<mlir::Type> taskReductionTypes;
-  for (const auto &taskreductionVar : clauseOps.taskReductionVars)
-    taskReductionTypes.push_back(taskreductionVar.getType());
-
-  // Add reduction variables as entry block arguments to the taskgroup region
-  llvm::SmallVector<mlir::Location> blockArgLocs(taskReductionSyms.size(), loc);
-  taskgroupOp->getRegion(0).addArguments(taskReductionTypes, blockArgLocs);
   return taskgroupOp;
 }
 

flang/test/Lower/OpenMP/task-inreduction.f90

@@ -32,4 +32,4 @@ subroutine omp_task_in_reduction()
    !$omp task in_reduction(+:i)
    i = i + 1
    !$omp end task
-end subroutine omp_task_in_reduction
+end subroutine omp_task_in_reduction

flang/test/Lower/OpenMP/taskgroup-task-array-reduction.f90

@@ -46,4 +46,4 @@ subroutine taskReduction(x)
    !$omp end task
    !$omp end taskgroup
    !$omp end parallel
-end subroutine
+end subroutine

flang/test/Lower/OpenMP/taskgroup-task_reduction01.f90

@@ -31,4 +31,4 @@ subroutine omp_taskgroup_task_reduction()
    !$omp taskgroup task_reduction(+:res)
    res = res + 1
    !$omp end taskgroup
-end subroutine omp_taskgroup_task_reduction
+end subroutine

flang/test/Lower/OpenMP/taskgroup-task_reduction02.f90

@@ -25,12 +25,12 @@
 !CHECK:          return
 !CHECK:         }
 
-subroutine in_reduction
+subroutine in_reduction()
    integer :: x
    x = 0
    !$omp taskgroup task_reduction(+:x)
    !$omp task in_reduction(+:x)
    x = x + 1
    !$omp end task
    !$omp end taskgroup
-end subroutine
+end subroutine