Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP of allowing OpenMP code to call genFIR from FirConverter #74653

Closed
wants to merge 1 commit into from

Conversation

kparzysz
Copy link
Contributor

@kparzysz kparzysz commented Dec 6, 2023

All commits squashed together to make it easier for me to rebase on top of main.

At the moment, this maintains status-quo, i.e. doesn't change any functionality. The genOMP functions don't yet call genFIR. check-flang passes for me, but there may be some bugs introduced when I manually rebased the code from an internal branch to main.

The main changes so far:

  • Separate the definition of the FirConverter class into its own header file, keep the member function implementations in Bridge.cpp.
  • Implement OpenMPMixin class, which provides (two) definitions of genFIR for OpenMP construct (+declarative construct). These two functions were originally in FirConverter, they were simply moved to the OpenMP mixin. The FirConverter class inherits the mixin class and injects these two functions into its own scope. The idea was to make the generic FIR generator as OpenMP-agnostic as possible.
  • Organize the OpenMP code into member functions of the mixin. Most constructs now have their own genOMP functions that serve a similar role to genFIR in the converter (i.e. to be visited by [this](auto &&x) { genOMP(x); } visitor).

Copy link

github-actions bot commented Dec 6, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff b4e19155171edf14412dc4132b7b10a84ac65fc5 3a737a391be73fc88749db8ed1675320961de684 -- flang/lib/Lower/FirConverter.h flang/lib/Lower/FirMixin.h flang/lib/Lower/OpenMPMixin.h flang/include/flang/Lower/OpenMP.h flang/lib/Lower/Bridge.cpp flang/lib/Lower/DirectivesCommon.h flang/lib/Lower/OpenMP.cpp
View the diff from clang-format here.
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index 03b3a30e8b..197de1b099 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -10,9 +10,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "flang/Lower/Bridge.h"
 #include "flang/Lower/AbstractConverter.h"
 #include "flang/Lower/Allocatable.h"
-#include "flang/Lower/Bridge.h"
 #include "flang/Lower/CallInterface.h"
 #include "flang/Lower/Coarray.h"
 #include "flang/Lower/ConvertCall.h"
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index eecd6d8e57..4f91ab54e3 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -586,7 +586,7 @@ void createEmptyRegionBlocks(
       if (eval.block->empty()) {
         eval.block->erase();
         eval.block = builder.createBlock(region);
-//abort();
+        // abort();
       } else {
         [[maybe_unused]] mlir::Operation &terminatorOp = eval.block->back();
         assert(mlir::isa<TerminatorOps...>(terminatorOp) &&
diff --git a/flang/lib/Lower/FirConverter.h b/flang/lib/Lower/FirConverter.h
index 34e2b1d241..e90e5c176e 100644
--- a/flang/lib/Lower/FirConverter.h
+++ b/flang/lib/Lower/FirConverter.h
@@ -506,7 +506,8 @@ private:
   void forceControlVariableBinding(const Fortran::semantics::Symbol *sym,
                                    mlir::Value inducVar);
 
-  template <typename A> void prepareExplicitSpace(const A &forall);
+  template <typename A>
+  void prepareExplicitSpace(const A &forall);
   void cleanupExplicitSpace();
 
   void genForallNest(const Fortran::parser::ConcurrentHeader &header);
@@ -569,7 +570,8 @@ private:
 
   // Is the insertion point of the builder directly or indirectly set
   // inside any operation of type "Op"?
-  template <typename... Op> bool isInsideOp() const;
+  template <typename... Op>
+  bool isInsideOp() const;
   bool isInsideHlfirForallOrWhere() const;
   bool isInsideHlfirWhere() const;
   void lowerWhereMaskToHlfir(mlir::Location loc,
@@ -604,7 +606,8 @@ private:
   /// position is updated automatically when visiting statements, but not when
   /// entering higher level nodes like constructs or procedures. This helper is
   /// intended to cover the latter cases.
-  template <typename A> void setCurrentPositionAt(const A &parseTreeNode);
+  template <typename A>
+  void setCurrentPositionAt(const A &parseTreeNode);
 
   mlir::Location toLocation(const Fortran::parser::CharBlock &cb);
   mlir::Location toLocation();
@@ -721,7 +724,8 @@ void FirConverter::genNestedStatement(
   genFIR(stmt.statement);
 }
 
-template <typename A> void FirConverter::prepareExplicitSpace(const A &forall) {
+template <typename A>
+void FirConverter::prepareExplicitSpace(const A &forall) {
   if (!explicitIterSpace.isActive())
     analyzeExplicitSpace(forall);
   localSymbols.pushScope();
@@ -785,7 +789,8 @@ void FirConverter::genIoConditionBranches(Fortran::lower::pft::Evaluation &eval,
 
 // Is the insertion point of the builder directly or indirectly set
 // inside any operation of type "Op"?
-template <typename... Op> bool FirConverter::isInsideOp() const {
+template <typename... Op>
+bool FirConverter::isInsideOp() const {
   mlir::Block *block = builder->getInsertionBlock();
   mlir::Operation *op = block ? block->getParentOp() : nullptr;
   while (op) {
diff --git a/flang/lib/Lower/FirMixin.h b/flang/lib/Lower/FirMixin.h
index b87f413769..e1e5601d86 100644
--- a/flang/lib/Lower/FirMixin.h
+++ b/flang/lib/Lower/FirMixin.h
@@ -3,7 +3,8 @@
 
 namespace Fortran::lower {
 
-template <typename FirConverterT> class FirMixinBase {
+template <typename FirConverterT>
+class FirMixinBase {
 public:
   FirConverterT *This() { return static_cast<FirConverterT *>(this); }
   const FirConverterT *This() const {
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 17b6ab3fae..ffb52ea925 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -10,12 +10,12 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "flang/Lower/OpenMP.h"
 #include "DirectivesCommon.h"
 #include "flang/Common/idioms.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/ConvertExpr.h"
 #include "flang/Lower/ConvertVariable.h"
-#include "flang/Lower/OpenMP.h"
 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Lower/StatementContext.h"
 #include "flang/Optimizer/Builder/BoxValue.h"
diff --git a/flang/lib/Lower/OpenMPMixin.h b/flang/lib/Lower/OpenMPMixin.h
index ddff14af4e..66408f6ff3 100644
--- a/flang/lib/Lower/OpenMPMixin.h
+++ b/flang/lib/Lower/OpenMPMixin.h
@@ -20,7 +20,7 @@ class SymMap;
 namespace pft {
 class Evaluation;
 class Variable;
-}
+} // namespace pft
 
 template <typename ConverterT>
 class OpenMPMixin : public FirMixinBase<ConverterT> {

@kparzysz kparzysz closed this Dec 15, 2023
@kparzysz kparzysz deleted the recurse-genfir branch December 15, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant