[flang] Fix processing ModuleLikeUnit evaluationList

[peixin]

Summary

Push the ModuleLikeUnit evalutionList when entering module unit. Pop it
when exiting module unit if there is no module procedure. Otherwise, pop
it when entering the first module procedure.

[peixin]

https://github.com/flang-compiler/f18-llvm-project/pull/1184 is upstreamed in D120336. I noticed the function code in https://github.com/flang-compiler/f18-llvm-project/pull/1184 is not fully tested and there is one bug. This and D120459 address those two problems.

[kiranchandramohan]

Can you check why the patch application fails?

flang/lib/Lower/PFTBuilder.cpp

321–322

What happens without this code?

[peixin]

In D120460#3347185, @kiranchandramohan wrote:

Can you check why the patch application fails?

This is based on D120459. I had thought the whole file will be applied when applying the patch in Phabricator. I need to rebase this after D120459. Maybe I can merge them if it is easy for review?

flang/lib/Lower/PFTBuilder.cpp

321–322

This function enterConstructOrDirective is called by main program unit, module procedure, internal procedure, external procedure. Only to pop the evalutionlist when entering the module procedure unit since the evalutionlist stack must be empty for OpenMP/OpenACC declarative directives when calling enterFunction. Check https://github.com/llvm/llvm-project/blob/f3480390be614604907be447afa3f5ab10b97d04/flang/lib/Lower/PFTBuilder.cpp#L245-L286.

To be more clear, there are two cases need to be noticed:

$ cat file1.f90
module m
  integer x
  !$omp threadprivate(x)
end module
$ cat file2.f90
module m
  integer x
  !$omp threadprivate(x)
contains
  subroutine sub()
  end
end module

For file1.f90, just need to process the evalutionlist as if it is used in main program unit or internal procedure and the evalutionlist is popped when ending the module. For file2.f90, the evalutionlist cannot be popped when ending the module. Instead, need to pop it before entering the subroutine sub.

[shraiysh]

In D120460#3347208, @peixin wrote:

In D120460#3347185, @kiranchandramohan wrote:

Can you check why the patch application fails?

This is based on D120459. I had thought the whole file will be applied when applying the patch in Phabricator. I need to rebase this after D120459. Maybe I can merge them if it is easy for review?

Added edge to that revision. This should help in patch application. For future ref, we can add a "Depends on DXXXXXX" line to the summary to automatically create this edge as mentioned here.

[peixin]

Added edge to that revision. This should help in patch application. For future ref, we can add a "Depends on DXXXXXX" line to the summary to automatically create this edge as mentioned here.

Got it. Thanks. Now CI passed.

[peixin]

The previous design for lowering OpenMP/OpenACC declarative constructs in module is dirty. Refactor the code as follows:

EnterModule (push ModuleLikeUnit evalutionList)

[Enter OpenMP/OpenACC decl constructs] (push declconstruct ... as other directives)
[Exit OpenMP/OpenACC decl constructs] (pop declconstruct ... as other directives)
[Enter module procedure] (pop ModuleLikeUnit evalutionList)
[Exit module procedure]

ExitModule (pop ModuleLikeUnit evalutionList if there is no module procedure)

[vdonaldson]

Looks ok to me.

[peixin]

Rebase to test CI before land.

[clementval]

Can you sync this change with fir-dev?

[peixin]

@clementval Sure. Will do it now.

flang/lib/Lower/PFTBuilder.cpp

public:
   }
 
   template <typename A>
   constexpr void Post(const A &) {
     if constexpr (lower::pft::isFunctionLike<A>) {
       exitFunction();
     } else if constexpr (lower::pft::isConstruct<A> ||
                          lower::pft::isDirective<A>) {
-      if constexpr (lower::pft::isDeclConstruct<A>)
-        return;
       exitConstructOrDirective();
     }
   }
 
   // Module like
   bool Pre(const parser::Module &node) { return enterModule(node); }
   bool Pre(const parser::Submodule &node) { return enterModule(node); }
 
private:
 
   /// Add the end statement Evaluation of a sub/program to the PFT.
   /// There may be intervening internal subprogram definitions between
   /// prior statements and this end statement.
   void endFunctionBody() {
     if (evaluationListStack.empty())
       return;
     auto evaluationList = evaluationListStack.back();
-    if (evaluationList->empty() &&
-        pftParentStack.back().getIf<lower::pft::ModuleLikeUnit>()) {
-      popEvaluationList();
-      return;
-    }
     if (evaluationList->empty() || !evaluationList->back().isEndStmt()) {
       const auto &endStmt =
           pftParentStack.back().get<lower::pft::FunctionLikeUnit>().endStmt;
       endStmt.visit(common::visitors{
           [&](const parser::Statement<parser::EndProgramStmt> &s) {
             addEvaluation(lower::pft::Evaluation{
                 s.statement, pftParentStack.back(), s.source, s.label});
           },
if (evaluationList->empty() || !evaluationList->back().isEndStmt()) {
             llvm::report_fatal_error("missing end statement or unexpected "
                                      "begin statement reference");
           },
       });
     }
     lastLexicalEvaluation = nullptr;
   }
 
+  /// Pop the ModuleLikeUnit evaluationList when entering the first module
+  /// procedure.
+  void cleanModuleEvaluationList() {
+    if (evaluationListStack.empty())
+      return;
+    if (pftParentStack.back().isA<lower::pft::ModuleLikeUnit>())
+      popEvaluationList();
+  }
+
   /// Initialize a new function-like unit and make it the builder's focus.
   template <typename A>
   bool enterFunction(const A &func,
                      const semantics::SemanticsContext &semanticsContext) {
+    cleanModuleEvaluationList();
     endFunctionBody(); // enclosing host subprogram body, if any
     Fortran::lower::pft::FunctionLikeUnit &unit =
         addFunction(lower::pft::FunctionLikeUnit{func, pftParentStack.back(),
                                                  semanticsContext});
     labelEvaluationMap = &unit.labelEvaluationMap;
     assignSymbolLabelMap = &unit.assignSymbolLabelMap;
     functionList = &unit.nestedFunctions;
     pushEvaluationList(&unit.evaluationList);
private:
   /// Initialize a new construct or directive and make it the builder's focus.
   template <typename A>
   bool enterConstructOrDirective(const A &constructOrDirective) {
     Fortran::lower::pft::Evaluation &eval = addEvaluation(
         lower::pft::Evaluation{constructOrDirective, pftParentStack.back()});
     eval.evaluationList.reset(new lower::pft::EvaluationList);
     pushEvaluationList(eval.evaluationList.get());
     pftParentStack.emplace_back(eval);
     constructAndDirectiveStack.emplace_back(&eval);
-    if constexpr (lower::pft::isDeclConstruct<A>) {
-      popEvaluationList();
-      pftParentStack.pop_back();
-      constructAndDirectiveStack.pop_back();
-      popEvaluationList();
-    }
     return true;
   }
 
   void exitConstructOrDirective() {
     rewriteIfGotos();
     auto *eval = constructAndDirectiveStack.back();
     if (eval->isExecutableDirective()) {
       // A construct at the end of an (unstructured) OpenACC or OpenMP
       // construct region must have an exit target inside the region.

flang/test/Lower/pre-fir-tree06.f90

 ! RUN: %flang_fc1 -fdebug-pre-fir-tree -fopenmp %s | FileCheck %s
 
 ! Test structure of the Pre-FIR tree with OpenMP declarative construct
 
 ! CHECK: ModuleLike
 module m
   real, dimension(10) :: x
   ! CHECK-NEXT: OpenMPDeclarativeConstruct
   !$omp threadprivate(x)
 end
 ! CHECK: End ModuleLike
 
+! CHECK: ModuleLike
+module m2
+  integer, save :: i
+  ! CHECK-NEXT: OpenMPDeclarativeConstruct
+  !$omp threadprivate(i)
+contains
+  subroutine sub()
+    i = 1;
+  end
+  subroutine sub2()
+    i = 2;
+  end
+end
+! CHECK: End ModuleLike
+
+! CHECK: Program main
+program main
+  real :: y
+  ! CHECK-NEXT: OpenMPDeclarativeConstruct
+  !$omp threadprivate(y)
+end
+! CHECK: End Program main
+
+! CHECK: Subroutine sub1
+subroutine sub1()
+  real, save :: p
+  ! CHECK-NEXT: OpenMPDeclarativeConstruct
+  !$omp threadprivate(p)
+end
+! CHECK: End Subroutine sub1
+
+! CHECK: Subroutine sub2
+subroutine sub2()
+  real, save :: q
+  ! CHECK-NEXT: OpenMPDeclarativeConstruct
+  !$omp threadprivate(q)
+contains
+  subroutine sub()
+  end
+end
+! CHECK: End Subroutine sub2
+