[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct #456

SouraVX · 2020-09-28T15:04:05Z

TODO:
Handle Attribute based clauses:

Default clause
ProcBind clause

schweitzpgi · 2020-09-28T20:18:20Z

flang/lib/Lower/OpenMP.cpp

@@ -21,6 +23,32 @@

 #define TODO() llvm_unreachable("not yet implemented")

+static const Fortran::parser::Name *
+getDesignatorNameIfDataRef(const Fortran::parser::Designator &designator) {
+  const auto *dataRef{std::get_if<Fortran::parser::DataRef>(&designator.u)};


LLVM/MLIR prefer type var = expression; style. For consistency with MLIR, we follow that style in lowering.

schweitzpgi · 2020-09-28T20:21:49Z

flang/lib/Lower/OpenMP.cpp

+      } else if (const auto &defaultClause =
+                     std::get_if<Fortran::parser::OmpDefaultClause>(
+                         &clause.u)) {
+        TODO();


We're moving to TODO("string") in lowering. The string can be anything, including "", of course. See Todo.h for more.

schweitzpgi · 2020-09-28T20:23:53Z

flang/lib/Lower/OpenMP.cpp

@@ -11,6 +11,8 @@
 //===----------------------------------------------------------------------===//

 #include "flang/Lower/OpenMP.h"
+#include "SymbolMap.h"


Is this include required? (I probably just missed it while skimming...)

Thanks! Yea, not needed. Must be residual of my other work.

clementval · 2020-09-28T20:44:47Z

flang/lib/Lower/OpenMP.cpp

+        defaultClauseOperand.dyn_cast_or_null<StringAttr>(),
+        privateClauseOperands, firstprivateClauseOperands, sharedClauseOperands,
+        copyinClauseOperands,
+        procBindClauseOperand.dyn_cast_or_null<StringAttr>());


You can move the handling of proc_bind and default clause after the op create so you can directly set attribute on it. The code below should get you there. Didn't build it so might need some changes. Also I guess there should be helper functions generated by MLIR TableGen to get the name of the Attr and not hardcoding it

Suggested change

procBindClauseOperand.dyn_cast_or_null<StringAttr>());

procBindClauseOperand.dyn_cast_or_null<StringAttr>());

// Clauses mapped to attributes.

for (const auto &clause : parallelOpClauseList.v) {

if (const auto &procBindClause =

std::get_if<Fortran::parser::OmpProcBindClause>(

&clause.u)) {

switch (procBindClause.v) {

case OmpProcBindClause::Type::Close:

parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_close);

break;

case OmpProcBindClause::Type::Master:

parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_master);

break;

case OmpProcBindClause::Type::Spread:

parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_spread);

break;

} else if (const auto &defaultClause =

std::get_if<Fortran::parser::OmpDefaultClause>(

&clause.u)) {

switch (defaultClause.v) {

case OmpDefaultClause::Type::Private:

parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultPrivate);

break;

case OmpDefaultClause::Type::Firstprivate:

parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultFirstprivate);

break;

case OmpDefaultClause::Type::Shared:

parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultShared);

break;

case OmpDefaultClause::Type::None:

parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultNone);

break;

}

}

}

}

Thanks for the suggestion!, I also looked for other solutions.
This one doesn't work since mlir::omp::ClauseDefault::ClauseDefaultNone doesn't implicitly converts to Attribute.

parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultNone);

Maybe these 2 attributes based clauses deserves a fresh look separately.

As I mentioned, there is probably helper functions to translate enum to string for that purpose. This should be in the tablegen generated file for the OpenMP dialect.

kiranchandramohan

+1 for the progress here. See a few comments inline.

kiranchandramohan · 2020-09-28T20:52:10Z

flang/lib/Lower/OpenMP.cpp

@@ -21,6 +23,32 @@

 #define TODO() llvm_unreachable("not yet implemented")

+static const Fortran::parser::Name *
+getDesignatorNameIfDataRef(const Fortran::parser::Designator &designator) {


Can this function be shared (with OpenACC, possibly elsewhere too) by putting it in a commonplace?

kiranchandramohan · 2020-09-28T20:53:01Z

flang/lib/Lower/OpenMP.cpp

+  return dataRef ? std::get_if<Fortran::parser::Name>(&dataRef->u) : nullptr;
+}
+
+static void genObjectList(const Fortran::parser::OmpObjectList &objectList,


Can this be shared too?

Yes, I'm observing and marking these. Some of the Directive handling code can be shared(Maybe LowerDirectives.cpp in lib/Lower dir ? or so..). But these are not on top of my priority list as of now.
Since they may break things(for short term) in Lowering of OpenACC or OpenMP of which both are developing rapidly and to some sort orthogonal to each other.

kiranchandramohan · 2020-09-28T21:03:07Z

flang/test/Lower/OpenMP/omp-parallel-if-clause.f90

+!LLVMIR:   call void @_QQmain..omp_par
+!LLVMIR:   call void @__kmpc_end_serialized_parallel
+
+program ifclause


Add a few more variants with true, false, constant eval logical expression, logical variable on the if clause.

kiranchandramohan · 2020-09-28T21:05:15Z

flang/test/Lower/OpenMP/omp-parallel-private-clause.f90

+!LLVMIRDialect:    omp.terminator
+!LLVMIRDialect:  }
+
+program private_clause


A few more variants,
-> subroutine arguments.
-> Array

Done thanks!
Just wanted clarify that array print inside a omp region doesn't work(crashes). So test cases captures array, however do not attempt to print them.
This may be related to:
#454
I think once we have the minimal set of operations lowered to fir dialect, we can start to focus on phase ordering/lower level issues. This will again help us to re-establish end-to-end functional test cases.

SouraVX · 2020-09-29T08:08:20Z

flang/lib/Lower/OpenMP.cpp

+      } else if (const auto &procBindClause =
+                     std::get_if<Fortran::parser::OmpProcBindClause>(
+                         &clause.u)) {
+        TODO("default clause for omp parallel");


Ah, I'll take care of this in subsequent revision.

If you don't handle these in this PR, can you adapt the name of the PR and description to reflect it.

Sure, makes sense! Actually just now I noticed there's one more clause allocate defined in 5.0 standard, currently in review stage.

Rin is handling the allocate clause. You can leave that for now.

clementval · 2020-09-29T14:08:39Z

flang/lib/Lower/OpenMP.cpp

+      } else if (const auto &procBindClause =
+                     std::get_if<Fortran::parser::OmpProcBindClause>(
+                         &clause.u)) {
+        TODO("default clause for omp parallel");


If you don't handle these in this PR, can you adapt the name of the PR and description to reflect it.

SouraVX · 2020-09-29T15:22:49Z

@schweitzpgi plan is to merge this PR post this week scheduled merge. Some changes are still in LLVM master.
I'll straighten up this PR and keep you posted.
Thanks!

…or Parallel construct TODO: Handle Attribute based clauses: 1. Default clause 2. ProcBind clause

SouraVX · 2020-10-02T20:08:20Z

Squashed and prepared for merging.

Note: This patch reflects the work that can be upstreamed from PR's(merged): 1. flang-compiler#456 2. flang-compiler#485 Also replaced TODO with new TODO. Reviewed By: kiranchandramohan Differential Revision: https://reviews.llvm.org/D89769

Note: This patch reflects the work that can be upstreamed from PR's(merged): 1. flang-compiler/f18-llvm-project#456 2. flang-compiler/f18-llvm-project#485 Also replaced TODO with new TODO. Reviewed By: kiranchandramohan Differential Revision: https://reviews.llvm.org/D89769

schweitzpgi requested review from schweitzpgi, clementval and kiranchandramohan September 28, 2020 20:16

schweitzpgi reviewed Sep 28, 2020

View reviewed changes

clementval reviewed Sep 28, 2020

View reviewed changes

kiranchandramohan approved these changes Sep 28, 2020

View reviewed changes

SouraVX requested review from schweitzpgi, clementval and kiranchandramohan September 29, 2020 08:04

SouraVX commented Sep 29, 2020

View reviewed changes

clementval requested changes Sep 29, 2020

View reviewed changes

SouraVX changed the title ~~[flang][OpenMP] Handle the remaining clauses for parallel construct~~ [flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct Sep 29, 2020

schweitzpgi force-pushed the fir-dev branch from 13c5674 to 34bbd11 Compare October 1, 2020 20:43

[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses f…

0f26635

…or Parallel construct TODO: Handle Attribute based clauses: 1. Default clause 2. ProcBind clause

schweitzpgi merged commit 9550c6e into flang-compiler:fir-dev Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct #456

[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct #456

SouraVX commented Sep 28, 2020

schweitzpgi Sep 28, 2020

schweitzpgi Sep 28, 2020 •

edited

Loading

schweitzpgi Sep 28, 2020

SouraVX Sep 29, 2020

clementval Sep 28, 2020

SouraVX Sep 29, 2020

clementval Sep 29, 2020

kiranchandramohan left a comment

kiranchandramohan Sep 28, 2020

kiranchandramohan Sep 28, 2020

SouraVX Sep 29, 2020

kiranchandramohan Sep 28, 2020

kiranchandramohan Sep 28, 2020

SouraVX Sep 29, 2020 •

edited

Loading

SouraVX Sep 29, 2020

clementval Sep 29, 2020

SouraVX Sep 29, 2020

kiranchandramohan Sep 29, 2020

clementval Sep 29, 2020

SouraVX commented Sep 29, 2020

SouraVX commented Oct 2, 2020

-        procBindClauseOperand.dyn_cast_or_null<StringAttr>());
+        procBindClauseOperand.dyn_cast_or_null<StringAttr>());
+        // Clauses mapped to attributes.
+        for (const auto &clause : parallelOpClauseList.v) {
+          if (const auto &procBindClause =
+                      std::get_if<Fortran::parser::OmpProcBindClause>(
+                          &clause.u)) {
+           switch (procBindClause.v) {
+           case OmpProcBindClause::Type::Close:
+             parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_close);
+             break;
+           case OmpProcBindClause::Type::Master:
+             parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_master);
+             break;
+           case OmpProcBindClause::Type::Spread:
+             parallelOp.setAttr("proc_bind_val", llvm::omp::OMP_PROC_BIND_spread);
+             break;
+           } else if (const auto &defaultClause =
+                      std::get_if<Fortran::parser::OmpDefaultClause>(
+                          &clause.u)) {
+             switch (defaultClause.v) {
+             case OmpDefaultClause::Type::Private:
+               parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultPrivate);
+               break;
+             case OmpDefaultClause::Type::Firstprivate:
+               parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultFirstprivate);
+               break;
+             case OmpDefaultClause::Type::Shared:
+               parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultShared);
+               break;
+             case OmpDefaultClause::Type::None:
+               parallelOp.setAttr("default_val", mlir::omp::ClauseDefault::ClauseDefaultNone);
+               break;
+             }
+           }
+         }
+       }

[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct #456

[flang][OpenMP] Lower private, firstprivate, shared, copyin clauses for Parallel construct #456

Conversation

SouraVX commented Sep 28, 2020

Choose a reason for hiding this comment

schweitzpgi Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiranchandramohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SouraVX Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SouraVX commented Sep 29, 2020

SouraVX commented Oct 2, 2020

schweitzpgi Sep 28, 2020 •

edited

Loading

SouraVX Sep 29, 2020 •

edited

Loading