[flang][OpenMP][WIP] Separate clause translation from directive translation

[shraiysh]

Summary

This patch separates clause translation from direction translation while converting F90 to OpenMP Dialect. This clause handler can be reused for other operations like wsloop, sections etc.

[clementval]

flang/lib/Lower/OpenMP.cpp

67

The name is a bit misleading. Something with OpenMP in the name would make more sense IMO.

[shraiysh]

Thanks for the review @clementval. Addressed comment.

[peixin]

flang/lib/Lower/OpenMP.cpp

67

How about removing MLIR? OpenMPClauses or OmpClauses?

71

We used someClauseOperand(s) for other constructs in OpenMP.cpp in fir-dev. Can we keep consistent with others?

74

handleProcBindClause -> genProcBindClause to be consistent with other function names in this file.

[shraiysh]

Thanks @peixin for the comments. I will rework on this once upstreaming of translations has been done. My plan is to remove individual clause handling from genOMP functions and put it in a collect() function so all the constructs can use it. I will ping this thread once the upstreaming is done.

[peixin]

flang/test/Lower/OpenMP/parallel.f90

3

I am thinking if flang_fc1 -fopenmp -emit-fir should be added. Check the comment in https://reviews.llvm.org/D122595. Also you can check the discussions in https://reviews.llvm.org/D121171#3370137. If new passes is added in flang-new, I am not sure if it will crash the test cases.

17

Please add CHECK-LABEL for each subroutine.

[shraiysh]

Added handling for block constructs, sections constructs and critical construct.

[peixin]

flang/lib/Lower/OpenMP.cpp

144

As I mentioned in D124138, there is one drawback collecting clauses in one place for all block constructs. For those unsupported clauses, one hard TODO is changed into one unknown result, maybe one segfault. It is easy to omit some features support and get users into trouble if there is segfault/ICE.

Can we let each construct have their own collect? Here is my recomendation:

struct OpenMPMLIRClauses {
    void genNumThreadsClause(
      const Fortran::parser::OmpClause &clause,
      mlir::Value &mlirNumThreadsExpr)
     void genIFClause(...)
  ...
}

if (blockDirective.v == llvm::omp::OMPD_parallel) {
  for (const Fortran::parser::OmpClause &clause : clauseList.v) {
    genNumThreadsClause(...); ! if lowering to LLVM IR has been supported.
    genIfClause(...); ! if lowering to LLVM IR has been supported.
    TODO(... clause unsupported for parallel construct);
  }
  ...
} else if (blockDirective.v == llvm::omp::OMPD_master) {

As we pursue the OpenMP standard, it is easily know what are not supported yet. And this refector patch won't need to wait for other patches.
@shraiysh @kiranchandramohan @clementval What do you think?

[peixin]

@shraiysh Can you restart this work? There are two small patches left to upstream and it won't be affected too much.

To refactor the code, there are several things that I can think of to handle:

1. Explicit clauses for each construct. One unsupported clause should be given one TODO.
2. The clauses which are not allowed in directives should be given one assert?
3. The combined constructs are not carefully organized.

flang/lib/Lower/OpenMP.cpp

144

I take this back. This cause the code duplication.

[shraiysh]

Sure @peixin, thank you for pinging here. I will restart this work soon and try to incorporate your ideas and concerns.

[skatrak]

Hi @shraiysh, @peixin. I just realized about this patch, thanks to @kiranchandramohan, after I spent time working on a patch with basically the same goal of refactoring the clause processing in the PFT to MLIR lowering. The other patch is here: D155981.

It follows a slightly different approach where there is no collect() method or state to be kept, so the responsibility of deciding which clauses to generate code for is down to the caller. I am interested to see if you have any comments to improve that approach before landing it. It does other nonfunctional changes apart from extracting the clause-processing logic into a class, so the diff can be a bit noisy.

[shraiysh]

Thank you for working on this @skatrak. I will abandon this patch now.

flang/lib/Lower/OpenMP.cpp

 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Lower/StatementContext.h"
 #include "flang/Lower/Todo.h"
 #include "flang/Optimizer/Builder/BoxValue.h"
 #include "flang/Optimizer/Builder/FIRBuilder.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/tools.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 
 using namespace mlir;
 
 static const Fortran::parser::Name *
 getDesignatorNameIfDataRef(const Fortran::parser::Designator &designator) {
   const auto *dataRef = std::get_if<Fortran::parser::DataRef>(&designator.u);
   return dataRef ? std::get_if<Fortran::parser::Name>(&dataRef->u) : nullptr;
static void createBodyOfOp(Op &op, fir::FirOpBuilder &firOpBuilder,
   auto &block = op.getRegion().back();
   firOpBuilder.setInsertionPointToStart(&block);
   // Ensure the block is well-formed.
   firOpBuilder.create<mlir::omp::TerminatorOp>(loc);
   // Reset the insertion point to the start of the first block.
   firOpBuilder.setInsertionPointToStart(&block);
 }
 
+struct MLIRClauses {
+  MLIRClauses(Fortran::lower::AbstractConverter &converter)
+      : converter(converter) {}
+
+  mlir::Value ifVal, numThreadsVal;
+  omp::ClauseProcBindKindAttr pbKindAttr;
+
+  void handleProcBindClause(const Fortran::parser::OmpClause::ProcBind &pbKind,
+                            omp::ClauseProcBindKindAttr &pbKindAttr) {
+    std::string pbStr =
+        Fortran::parser::OmpProcBindClause::EnumToString(pbKind.v.v);
+    std::transform(pbStr.begin(), pbStr.end(), pbStr.begin(), llvm::toLower);
+    auto pbVal = mlir::omp::symbolizeClauseProcBindKind(pbStr);
+    if (pbVal) {
+      pbKindAttr = omp::ClauseProcBindKindAttr::get(
+          converter.getFirOpBuilder().getContext(), *pbVal);
+    }
+  }
+
+  void collect(const Fortran::parser::OmpClauseList &clauseList) {
+    for (const Fortran::parser::OmpClause &clause : clauseList.v) {
+      std::visit(
+          Fortran::common::visitors{
+              [&](const Fortran::parser::OmpClause::If &ifClause) {
+                auto &expr =
+                    std::get<Fortran::parser::ScalarLogicalExpr>(ifClause.v.t);
+                ifVal = fir::getBase(converter.genExprValue(
+                    *Fortran::semantics::GetExpr(expr), stmtCtx));
+              },
+              [&](const Fortran::parser::OmpClause::NumThreads
+                      &numThreadsClause) {
+                numThreadsVal = fir::getBase(converter.genExprValue(
+                    *Fortran::semantics::GetExpr(numThreadsClause.v), stmtCtx));
+              },
+              [&](const Fortran::parser::OmpClause::ProcBind &pbKind) {
+                handleProcBindClause(pbKind, pbKindAttr);
+              },
+              [](const auto &) {}},
+          clause.u);
+    }
+  }
+
+private:
+  Fortran::lower::StatementContext stmtCtx;
+  Fortran::lower::AbstractConverter &converter;
+};
+
 static void genOMP(Fortran::lower::AbstractConverter &converter,
                    Fortran::lower::pft::Evaluation &eval,
                    const Fortran::parser::OpenMPSimpleStandaloneConstruct
                        &simpleStandaloneConstruct) {
   const auto &directive =
       std::get<Fortran::parser::OmpSimpleStandaloneDirective>(
           simpleStandaloneConstruct.t);
   switch (directive.v) {
static void genOMP(Fortran::lower::AbstractConverter &converter,
   case llvm::omp::Directive::OMPD_target_enter_data:
     TODO(converter.getCurrentLocation(), "OMPD_target_enter_data");
   case llvm::omp::Directive::OMPD_target_exit_data:
     TODO(converter.getCurrentLocation(), "OMPD_target_exit_data");
   case llvm::omp::Directive::OMPD_target_update:
     TODO(converter.getCurrentLocation(), "OMPD_target_update");
   case llvm::omp::Directive::OMPD_ordered:
     TODO(converter.getCurrentLocation(), "OMPD_ordered");
   }
 }
 
 static void
 genOMP(Fortran::lower::AbstractConverter &converter,
        Fortran::lower::pft::Evaluation &eval,
        const Fortran::parser::OpenMPStandaloneConstruct &standaloneConstruct) {
   std::visit(
       Fortran::common::visitors{
genOMP(Fortran::lower::AbstractConverter &converter,
        const Fortran::parser::OpenMPBlockConstruct &blockConstruct) {
   const auto &beginBlockDirective =
       std::get<Fortran::parser::OmpBeginBlockDirective>(blockConstruct.t);
   const auto &blockDirective =
       std::get<Fortran::parser::OmpBlockDirective>(beginBlockDirective.t);
 
   auto &firOpBuilder = converter.getFirOpBuilder();
   auto currentLocation = converter.getCurrentLocation();
-  Fortran::lower::StatementContext stmtCtx;
   llvm::ArrayRef<mlir::Type> argTy;
-  if (blockDirective.v == llvm::omp::OMPD_parallel) {
-
-    mlir::Value ifClauseOperand, numThreadsClauseOperand;
-    Attribute procBindClauseOperand;
 
+  if (blockDirective.v == llvm::omp::OMPD_parallel) {
     const auto &parallelOpClauseList =
         std::get<Fortran::parser::OmpClauseList>(beginBlockDirective.t);
-    for (const auto &clause : parallelOpClauseList.v) {
-      if (const auto &ifClause =
-              std::get_if<Fortran::parser::OmpClause::If>(&clause.u)) {
-        auto &expr =
-            std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
-        ifClauseOperand = fir::getBase(converter.genExprValue(
-            *Fortran::semantics::GetExpr(expr), stmtCtx));
-      } else if (const auto &numThreadsClause =
-                     std::get_if<Fortran::parser::OmpClause::NumThreads>(
-                         &clause.u)) {
-        // OMPIRBuilder expects `NUM_THREAD` clause as a `Value`.
-        numThreadsClauseOperand = fir::getBase(converter.genExprValue(
-            *Fortran::semantics::GetExpr(numThreadsClause->v), stmtCtx));
-      }
-      // TODO: Handle private, firstprivate, shared and copyin
-    }
+    MLIRClauses mlirClauses(converter);
+    mlirClauses.collect(parallelOpClauseList);
     // Create and insert the operation.
     auto parallelOp = firOpBuilder.create<mlir::omp::ParallelOp>(
-        currentLocation, argTy, ifClauseOperand, numThreadsClauseOperand,
-        ValueRange(), ValueRange(),
-        procBindClauseOperand.dyn_cast_or_null<omp::ClauseProcBindKindAttr>());
-    // Handle attribute based clauses.
-    for (const auto &clause : parallelOpClauseList.v) {
-      // TODO: Handle default clause
-      if (const auto &procBindClause =
-              std::get_if<Fortran::parser::OmpClause::ProcBind>(&clause.u)) {
-        const auto &ompProcBindClause{procBindClause->v};
-        omp::ClauseProcBindKind pbKind;
-        switch (ompProcBindClause.v) {
-        case Fortran::parser::OmpProcBindClause::Type::Master:
-          pbKind = omp::ClauseProcBindKind::Master;
-          break;
-        case Fortran::parser::OmpProcBindClause::Type::Close:
-          pbKind = omp::ClauseProcBindKind::Close;
-          break;
-        case Fortran::parser::OmpProcBindClause::Type::Spread:
-          pbKind = omp::ClauseProcBindKind::Spread;
-          break;
-        }
-        parallelOp.proc_bind_valAttr(omp::ClauseProcBindKindAttr::get(
-            firOpBuilder.getContext(), pbKind));
-      }
-    }
+        currentLocation, argTy, mlirClauses.ifVal, mlirClauses.numThreadsVal,
+        ValueRange(), ValueRange(), mlirClauses.pbKindAttr);
     createBodyOfOp<omp::ParallelOp>(parallelOp, firOpBuilder, currentLocation);
   } else if (blockDirective.v == llvm::omp::OMPD_master) {
     auto masterOp =
         firOpBuilder.create<mlir::omp::MasterOp>(currentLocation, argTy);
     createBodyOfOp<omp::MasterOp>(masterOp, firOpBuilder, currentLocation);
   }
 }
 

flang/test/Lower/OpenMP/parallel.f90

+! RUN: bbc -fopenmp -emit-fir -o - %s | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -emit-fir -o - %s | FileCheck %s
+
+subroutine parallel_simple()
+  ! CHECK: omp.parallel
+!$omp parallel
+  ! CHECK: fir.call
+  call f1()
+!$omp end parallel
+end subroutine parallel_simple
+
+!===============================================================================
+! `if` clause
+!===============================================================================
+
+subroutine parallel_if(alpha)
+  integer, intent(in) :: alpha
+
+  ! CHECK: omp.parallel if(%{{.*}} : i1) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel if(alpha .le. 0)
+  call f1()
+!$omp end parallel
+
+  ! CHECK: omp.parallel if(%{{.*}} : i1) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel if(.false.)
+  call f2()
+!$omp end parallel
+
+  ! CHECK: omp.parallel if(%{{.*}} : i1) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel if(alpha .ge. 0)
+  call f3()
+!$omp end parallel
+
+  ! CHECK: omp.parallel if(%{{.*}} : i1) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel if(.true.)
+  call f4()
+!$omp end parallel
+
+!$ end subroutine parallel_if
+
+!===============================================================================
+! `num_threads` clause
+!===============================================================================
+
+subroutine parallel_numthreads(num_threads)
+  integer, intent(inout) :: num_threads
+
+  ! CHECK: omp.parallel num_threads(%{{.*}}: i32) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel num_threads(16)
+  call f1()
+!$omp end parallel
+
+  num_threads = 4
+
+  ! CHECK: omp.parallel num_threads(%{{.*}} : i32) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel num_threads(num_threads)
+  call f2()
+!$omp end parallel
+
+end subroutine parallel_numthreads
+
+!===============================================================================
+! `proc_bind` clause
+!===============================================================================
+
+subroutine parallel_proc_bind()
+
+  ! CHECK: omp.parallel proc_bind(master) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel proc_bind(master)
+  call f1()
+!$omp end parallel
+
+  ! CHECK: omp.parallel proc_bind(close) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel proc_bind(close)
+  call f2()
+!$omp end parallel
+
+  ! CHECK: omp.parallel proc_bind(spread) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel proc_bind(spread)
+  call f3()
+!$omp end parallel
+
+end subroutine parallel_proc_bind
+
+!===============================================================================
+! multiple clauses
+!===============================================================================
+
+subroutine parallel_multiple_clauses(alpha, num_threads)
+  integer, intent(in) :: alpha
+  integer, intent(in) :: num_threads
+
+  ! CHECK: omp.parallel if({{.*}} : i1) proc_bind(master) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel if(alpha .le. 0) proc_bind(master)
+  call f1()
+!$omp end parallel
+
+  ! CHECK: omp.parallel num_threads({{.*}} : i32) proc_bind(close) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel proc_bind(close) num_threads(num_threads)
+  call f2()
+!$omp end parallel
+
+  ! CHECK: omp.parallel if({{.*}} : i1) num_threads({{.*}} : i32) {
+  ! CHECK-NEXT: fir.call
+  ! CHECK-NEXT: omp.terminator
+!$omp parallel num_threads(num_threads) if(alpha .le. 0)
+  call f3()
+!$omp end parallel
+
+end subroutine parallel_multiple_clauses