[Flang][OpenMP] Finalize Statement context of clauses before op creation

[kiranchandramohan]

Currently Statement context used during processing of clauses is finalized after the OpenMP operation creation. This causes the finalization to go inside the OpenMP region and this can lead to multiple finalizations in a parallel region. This fix proposes to finalize them before the OpenMP op creation.

Fixes #71342

[llvmbot]

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Kiran Chandramohan (kiranchandramohan)

Changes

Currently Statement context used during processing of clauses is finalized after the OpenMP operation creation. This causes the finalization to go inside the OpenMP region and this can lead to multiple finalizations in a parallel region. This fix proposes to finalize them before the OpenMP op creation.

Note 1: The fix only does this for if clause, it can be extended to others and OpenACC if the approach is fine.
Note 2: The alternative is to finalize after and outside the OpenMP op region.

Fixes #71342

---

Full diff: https://github.com/llvm/llvm-project/pull/71679.diff

2 Files Affected:

• (modified) flang/lib/Lower/OpenMP.cpp (+9-16)
• (added) flang/test/Lower/OpenMP/parallel-if.f90 (+12)

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index c2568c629e521c1..315d1ea656ed0e7 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -561,8 +561,7 @@ class ClauseProcessor {
   bool processDepend(llvm::SmallVectorImpl<mlir::Attribute> &dependTypeOperands,
                      llvm::SmallVectorImpl<mlir::Value> &dependOperands) const;
   bool
-  processIf(Fortran::lower::StatementContext &stmtCtx,
-            Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName,
+  processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName,
             mlir::Value &result) const;
   bool
   processLink(llvm::SmallVectorImpl<DeclareTargetCapturePair> &result) const;
@@ -1672,9 +1671,9 @@ bool ClauseProcessor::processDepend(
 }
 
 bool ClauseProcessor::processIf(
-    Fortran::lower::StatementContext &stmtCtx,
     Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName,
     mlir::Value &result) const {
+  Fortran::lower::StatementContext stmtCtx;
   bool found = false;
   findRepeatableClause<ClauseTy::If>(
       [&](const ClauseTy::If *ifClause,
@@ -2305,8 +2304,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<mlir::Attribute> reductionDeclSymbols;
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx,
-               Fortran::parser::OmpIfClause::DirectiveNameModifier::Parallel,
+  cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::Parallel,
                ifClauseOperand);
   cp.processNumThreads(stmtCtx, numThreadsClauseOperand);
   cp.processProcBind(procBindKindAttr);
@@ -2359,8 +2357,7 @@ genTaskOp(Fortran::lower::AbstractConverter &converter,
       dependOperands;
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx,
-               Fortran::parser::OmpIfClause::DirectiveNameModifier::Task,
+  cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::Task,
                ifClauseOperand);
   cp.processAllocate(allocatorOperands, allocateOperands);
   cp.processDefault();
@@ -2417,8 +2414,7 @@ genDataOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<const Fortran::semantics::Symbol *> useDeviceSymbols;
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx,
-               Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetData,
+  cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetData,
                ifClauseOperand);
   cp.processDevice(stmtCtx, deviceOperand);
   cp.processUseDevicePtr(devicePtrOperands, useDeviceTypes, useDeviceLocs,
@@ -2463,7 +2459,7 @@ genEnterExitDataOp(Fortran::lower::AbstractConverter &converter,
   }
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx, directiveName, ifClauseOperand);
+  cp.processIf(directiveName, ifClauseOperand);
   cp.processDevice(stmtCtx, deviceOperand);
   cp.processNowait(nowaitAttr);
   cp.processMap(currentLocation, directive, semanticsContext, stmtCtx,
@@ -2587,8 +2583,7 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<const Fortran::semantics::Symbol *> mapSymbols;
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx,
-               Fortran::parser::OmpIfClause::DirectiveNameModifier::Target,
+  cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::Target,
                ifClauseOperand);
   cp.processDevice(stmtCtx, deviceOperand);
   cp.processThreadLimit(stmtCtx, threadLimitOperand);
@@ -2742,8 +2737,7 @@ genTeamsOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<mlir::Attribute> reductionDeclSymbols;
 
   ClauseProcessor cp(converter, clauseList);
-  cp.processIf(stmtCtx,
-               Fortran::parser::OmpIfClause::DirectiveNameModifier::Teams,
+  cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::Teams,
                ifClauseOperand);
   cp.processAllocate(allocatorOperands, allocateOperands);
   cp.processDefault();
@@ -3018,8 +3012,7 @@ static void genOMP(Fortran::lower::AbstractConverter &converter,
     llvm::SmallVector<mlir::Value> alignedVars, nontemporalVars;
     mlir::Value ifClauseOperand;
     mlir::IntegerAttr simdlenClauseOperand, safelenClauseOperand;
-    cp.processIf(stmtCtx,
-                 Fortran::parser::OmpIfClause::DirectiveNameModifier::Simd,
+    cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::Simd,
                  ifClauseOperand);
     cp.processSimdlen(simdlenClauseOperand);
     cp.processSafelen(safelenClauseOperand);
diff --git a/flang/test/Lower/OpenMP/parallel-if.f90 b/flang/test/Lower/OpenMP/parallel-if.f90
new file mode 100644
index 000000000000000..661bc0e46598d95
--- /dev/null
+++ b/flang/test/Lower/OpenMP/parallel-if.f90
@@ -0,0 +1,12 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK-LABEL: func @_QPtest1
+subroutine test1(a)
+integer :: a(:,:)
+!CHECK: hlfir.destroy
+!CHECK: omp.parallel if
+!$omp parallel if(any(a .eq. 1))
+!CHECK-NOT: hlfir.destroy
+  print *, "Hello"
+!$omp end parallel
+end subroutine

[skatrak]

LGTM. It looks to me like it would probably be a good idea to do this for others if not most or all of the other clauses. During my refactoring work I think I just tried to keep the original behavior, but I wasn't too familiar with the effects of these StatementContext objects, so there may be other related issues.

[jeanPerier]

Thanks Kiran, looks good. You could even push the StatementContext to be local to the getIfClauseOperand helper I think.

[vdonaldson]

@schweitzpgi 's StatementContext mechanism has proven to be quite useful in a variety of contexts. By now this includes construct and function scopes (which do have associated statements). They are really simple to use in most cases, and general enough to support additional use cases with explicit management if needed. Once you realize that, they can be tailored to specific cases as here. Looks good to me.