[NFC][flang][OpenMP] Refactor the privatization positions

[peixin]

Summary

The previous implementation performs the privatization in the inner
operation, which is actually incorrect. The privatization position is
case-by-case issue. For example, if the private and firstprivate are
both in the combined construct of parallel worksharing-loop simd. The
private is for SIMD construct, the firstprivate is for worksharing-loop
construct. Add the clause set data type so that the clauses
implementation can be explicitly placed.

[peixin]

Fix one typo.

[NimishMishra]

This might require a rebase

[peixin]

Thanks @NimishMishra for the notice. Will do it later.

flang/lib/Lower/OpenMP.cpp

 #include "flang/Optimizer/Builder/BoxValue.h"
 #include "flang/Optimizer/Builder/FIRBuilder.h"
 #include "flang/Optimizer/Builder/Todo.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/tools.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 
+using OmpClauseSet =
+    Fortran::common::EnumSet<llvm::omp::Clause, llvm::omp::Clause_enumSize>;
+
 using namespace mlir;
 
 int64_t Fortran::lower::getCollapseValue(
     const Fortran::parser::OmpClauseList &clauseList) {
   for (const auto &clause : clauseList.v) {
     if (const auto &collapseClause =
             std::get_if<Fortran::parser::OmpClause::Collapse>(&clause.u)) {
       const auto *expr = Fortran::semantics::GetExpr(collapseClause->v);
if constexpr (std::is_same_v<T, Fortran::parser::OmpClause::Firstprivate>) {
       bool success = converter.createHostAssociateVarClone(*sym);
       (void)success;
       assert(success && "Privatization failed due to existing binding");
     }
   }
 }
 
 static void privatizeVars(Fortran::lower::AbstractConverter &converter,
-                          const Fortran::parser::OmpClauseList &opClauseList) {
+                          const Fortran::parser::OmpClauseList &opClauseList,
+                          const OmpClauseSet &set) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   auto insPt = firOpBuilder.saveInsertionPoint();
   firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
   for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
     if (const auto &privateClause =
             std::get_if<Fortran::parser::OmpClause::Private>(&clause.u)) {
+      if (set.test(llvm::omp::Clause::OMPC_private))
         createPrivateVarSyms(converter, privateClause);
     } else if (const auto &firstPrivateClause =
                    std::get_if<Fortran::parser::OmpClause::Firstprivate>(
                        &clause.u)) {
+      if (set.test(llvm::omp::Clause::OMPC_firstprivate))
         createPrivateVarSyms(converter, firstPrivateClause);
     }
   }
   firOpBuilder.restoreInsertionPoint(insPt);
 }
 
 /// The COMMON block is a global structure. \p commonValue is the base address
 /// of the the COMMON block. As the offset from the symbol \p sym, generate the
 /// COMMON block member value (commonValue + offset) for the symbol.
if (eval.block) {
                "expected terminator op");
       }
     }
     if (!eval.isDirective() && eval.hasNestedEvaluations())
       createEmptyRegionBlocks(firOpBuilder, eval.getNestedEvaluations());
   }
 }
 
-/// Create the body (block) for an OpenMP Operation.
+/// Create the body (block) for an OpenMP Operation, which is an non-combined
+/// construct or one part of combined construct.
 ///
 /// \param [in]    op - the operation the body belongs to.
 /// \param [inout] converter - converter to use for the clauses.
 /// \param [in]    loc - location in source code.
 /// \param [in]    eval - current PFT node/evaluation.
 /// \oaran [in]    clauses - list of clauses to process.
 /// \param [in]    args - block arguments (induction variable[s]) for the
 ////                      region.
 /// \param [in]    outerCombined - is this an outer operation - prevents
 ///                                privatization.
+/// \param [in]    set - set of clauses to be implemented in this \p op.
 template <typename Op>
 static void
 createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
                mlir::Location &loc, Fortran::lower::pft::Evaluation &eval,
                const Fortran::parser::OmpClauseList *clauses = nullptr,
                const SmallVector<const Fortran::semantics::Symbol *> &args = {},
-               bool outerCombined = false) {
+               bool outerCombined = false, const OmpClauseSet &set = {}) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   // If an argument for the region is provided then create the block with that
   // argument. Also update the symbol's address with the mlir argument value.
   // e.g. For loops the argument is the induction variable. And all further
   // uses of the induction variable should use this mlir value.
   mlir::Operation *storeOp = nullptr;
   if (args.size()) {
     std::size_t loopVarTypeSize = 0;
createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
   }
 
   // Reset the insert point to before the terminator.
   if (storeOp)
     firOpBuilder.setInsertionPointAfter(storeOp);
   else
     firOpBuilder.setInsertionPointToStart(&block);
 
-  // Handle privatization. Do not privatize if this is the outer operation.
-  if (clauses && !outerCombined)
-    privatizeVars(converter, *clauses);
+  // Handle privatization.
+  if (clauses && !set.empty() &&
+      (set.test(llvm::omp::Clause::OMPC_private) ||
+       set.test(llvm::omp::Clause::OMPC_firstprivate)))
+    privatizeVars(converter, *clauses, set);
 
   if (std::is_same_v<Op, omp::ParallelOp>)
     threadPrivatizeVars(converter, eval);
 }
 
 static void genOMP(Fortran::lower::AbstractConverter &converter,
                    Fortran::lower::pft::Evaluation &eval,
                    const Fortran::parser::OpenMPSimpleStandaloneConstruct

  * create the parallel operation. It handles the parallel specific clauses
  * and leaves the rest for handling at the inner operations.
  * TODO: Refactor clause handling
  */
 template <typename Directive>
 static void
 createCombinedParallelOp(Fortran::lower::AbstractConverter &converter,
                          Fortran::lower::pft::Evaluation &eval,
-                         const Directive &directive) {
+                         const Directive &directive,
+                         const OmpClauseSet &set = {}) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   mlir::Location currentLocation = converter.getCurrentLocation();
   Fortran::lower::StatementContext stmtCtx;
   llvm::ArrayRef<mlir::Type> argTy;
   mlir::Value ifClauseOperand, numThreadsClauseOperand;
   SmallVector<Value> allocatorOperands, allocateOperands;
   mlir::omp::ClauseProcBindKindAttr procBindKindAttr;
   const auto &opClauseList =
genOMP(Fortran::lower::AbstractConverter &converter,
   }
 
   for (const auto &clause :
        std::get<Fortran::parser::OmpClauseList>(endBlockDirective.t).v) {
     if (std::get_if<Fortran::parser::OmpClause::Nowait>(&clause.u))
       nowaitAttr = firOpBuilder.getUnitAttr();
   }
 
+  OmpClauseSet privatizeSet{llvm::omp::Clause::OMPC_private,
+                            llvm::omp::Clause::OMPC_firstprivate};
   if (blockDirective.v == llvm::omp::OMPD_parallel) {
     // Create and insert the operation.
     auto parallelOp = firOpBuilder.create<mlir::omp::ParallelOp>(
         currentLocation, argTy, ifClauseOperand, numThreadsClauseOperand,
         allocateOperands, allocatorOperands, /*reduction_vars=*/ValueRange(),
         /*reductions=*/nullptr, procBindKindAttr);
     createBodyOfOp<omp::ParallelOp>(parallelOp, converter, currentLocation,
-                                    eval, &opClauseList);
+                                    eval, &opClauseList, /*iv=*/{},
+                                    /*isCombined=*/false, privatizeSet);
   } else if (blockDirective.v == llvm::omp::OMPD_master) {
     auto masterOp =
         firOpBuilder.create<mlir::omp::MasterOp>(currentLocation, argTy);
     createBodyOfOp<omp::MasterOp>(masterOp, converter, currentLocation, eval);
   } else if (blockDirective.v == llvm::omp::OMPD_single) {
     auto singleOp = firOpBuilder.create<mlir::omp::SingleOp>(
         currentLocation, allocateOperands, allocatorOperands, nowaitAttr);
     createBodyOfOp<omp::SingleOp>(singleOp, converter, currentLocation, eval);
   } else if (blockDirective.v == llvm::omp::OMPD_ordered) {
     auto orderedOp = firOpBuilder.create<mlir::omp::OrderedRegionOp>(
         currentLocation, /*simd=*/nullptr);
     createBodyOfOp<omp::OrderedRegionOp>(orderedOp, converter, currentLocation,
                                          eval);
   } else if (blockDirective.v == llvm::omp::OMPD_task) {
     auto taskOp = firOpBuilder.create<mlir::omp::TaskOp>(
         currentLocation, ifClauseOperand, finalClauseOperand, untiedAttr,
         mergeableAttr, /*in_reduction_vars=*/ValueRange(),
         /*in_reductions=*/nullptr, priorityClauseOperand, allocateOperands,
         allocatorOperands);
-    createBodyOfOp(taskOp, converter, currentLocation, eval, &opClauseList);
+    createBodyOfOp(taskOp, converter, currentLocation, eval, &opClauseList,
+                   /*iv=*/{}, /*isCombined=*/false, privatizeSet);
   } else {
     TODO(converter.getCurrentLocation(), "Unhandled block directive");
   }
 }
 
 static mlir::omp::ScheduleModifier
 translateModifier(const Fortran::parser::OmpScheduleModifierType &m) {
   switch (m.v) {
static void genOMP(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<mlir::Value> lowerBound, upperBound, step, linearVars,
       linearStepVars, reductionVars;
   mlir::Value scheduleChunkClauseOperand;
   mlir::Attribute scheduleClauseOperand, collapseClauseOperand,
       noWaitClauseOperand, orderedClauseOperand, orderClauseOperand;
   const auto &loopOpClauseList = std::get<Fortran::parser::OmpClauseList>(
       std::get<Fortran::parser::OmpBeginLoopDirective>(loopConstruct.t).t);
 
+  OmpClauseSet privatizeSet{llvm::omp::Clause::OMPC_private,
+                            llvm::omp::Clause::OMPC_firstprivate};
+
   const auto ompDirective =
       std::get<Fortran::parser::OmpLoopDirective>(
           std::get<Fortran::parser::OmpBeginLoopDirective>(loopConstruct.t).t)
           .v;
   if (llvm::omp::OMPD_parallel_do == ompDirective) {
     createCombinedParallelOp<Fortran::parser::OmpBeginLoopDirective>(
         converter, eval,
         std::get<Fortran::parser::OmpBeginLoopDirective>(loopConstruct.t));
static void genOMP(Fortran::lower::AbstractConverter &converter,
 
   // 2.9.3.1 SIMD construct
   // TODO: Support all the clauses
   if (llvm::omp::OMPD_simd == ompDirective) {
     TypeRange resultType;
     auto SimdLoopOp = firOpBuilder.create<mlir::omp::SimdLoopOp>(
         currentLocation, resultType, lowerBound, upperBound, step);
     createBodyOfOp<omp::SimdLoopOp>(SimdLoopOp, converter, currentLocation,
-                                    eval, &loopOpClauseList, iv);
+                                    eval, &loopOpClauseList, iv,
+                                    /*isCombined=*/false, privatizeSet);
     return;
   }
 
   // FIXME: Add support for following clauses:
   // 1. linear
   // 2. order
   auto wsLoopOp = firOpBuilder.create<mlir::omp::WsLoopOp>(
       currentLocation, lowerBound, upperBound, step, linearVars, linearStepVars,
if (const auto &endClauseList =
     const auto &clauseList =
         std::get<Fortran::parser::OmpClauseList>((*endClauseList).t);
     for (const Fortran::parser::OmpClause &clause : clauseList.v)
       if (std::get_if<Fortran::parser::OmpClause::Nowait>(&clause.u))
         wsLoopOp.nowaitAttr(firOpBuilder.getUnitAttr());
   }
 
   createBodyOfOp<omp::WsLoopOp>(wsLoopOp, converter, currentLocation, eval,
-                                &loopOpClauseList, iv);
+                                &loopOpClauseList, iv, /*isCombined=*/false,
+                                privatizeSet);
 }
 
 static void
 genOMP(Fortran::lower::AbstractConverter &converter,
        Fortran::lower::pft::Evaluation &eval,
        const Fortran::parser::OpenMPCriticalConstruct &criticalConstruct) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   mlir::Location currentLocation = converter.getCurrentLocation();