[Flang][Lower] NFC: Introduce SymMapScope helper class

[skatrak]

This patch creates a simple RAII wrapper class for SymMap to make it easier to use and prevent a missing matching popScope() for a pushScope() call on simple use cases.

Some push-pop pairs are replaced with instances of the new class by this patch.

[llvmbot]

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

Author: Sergio Afonso (skatrak)

Changes

This patch creates a simple RAII wrapper class for SymMap to make it easier to use and prevent a missing matching popScope() for a pushScope() call on simple use cases.

Some push-pop pairs are replaced with instances of the new class by this patch.

---

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

4 Files Affected:

• (modified) flang/include/flang/Lower/SymbolMap.h (+10)
• (modified) flang/lib/Lower/Bridge.cpp (+3-6)
• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+4-11)
• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+2-4)

diff --git a/flang/include/flang/Lower/SymbolMap.h b/flang/include/flang/Lower/SymbolMap.h
index a55e4b133fe0a8..c03f9afd40801c 100644
--- a/flang/include/flang/Lower/SymbolMap.h
+++ b/flang/include/flang/Lower/SymbolMap.h
@@ -334,6 +334,16 @@ class SymMap {
   llvm::SmallVector<std::pair<AcDoVar, mlir::Value>> impliedDoStack;
 };
 
+/// RAII wrapper for SymMap.
+class SymMapScope {
+public:
+  explicit SymMapScope(SymMap &map) : map(map) { map.pushScope(); }
+  ~SymMapScope() { map.popScope(); }
+
+private:
+  SymMap &map;
+};
+
 } // namespace Fortran::lower
 
 #endif // FORTRAN_LOWER_SYMBOLMAP_H
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index 7cdecb788425a0..f78e952e4782c7 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -2597,14 +2597,12 @@ class FirConverter : public Fortran::lower::AbstractConverter {
             stmt.t)
             .value();
     if (lowerToHighLevelFIR()) {
-      mlir::OpBuilder::InsertPoint insertPt = builder->saveInsertionPoint();
-      localSymbols.pushScope();
+      mlir::OpBuilder::InsertionGuard guard(*builder);
+      Fortran::lower::SymMapScope scope(localSymbols);
       genForallNest(concurrentHeader);
       genFIR(std::get<Fortran::parser::UnlabeledStatement<
                  Fortran::parser::ForallAssignmentStmt>>(stmt.t)
                  .statement);
-      localSymbols.popScope();
-      builder->restoreInsertionPoint(insertPt);
       return;
     }
     prepareExplicitSpace(stmt);
@@ -2824,7 +2822,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
   }
 
   void genFIR(const Fortran::parser::CUFKernelDoConstruct &kernel) {
-    localSymbols.pushScope();
+    Fortran::lower::SymMapScope scope(localSymbols);
     const Fortran::parser::CUFKernelDoConstruct::Directive &dir =
         std::get<Fortran::parser::CUFKernelDoConstruct::Directive>(kernel.t);
 
@@ -3015,7 +3013,6 @@ class FirConverter : public Fortran::lower::AbstractConverter {
 
     builder->create<fir::FirEndOp>(loc);
     builder->setInsertionPointAfter(op);
-    localSymbols.popScope();
   }
 
   void genFIR(const Fortran::parser::OpenMPConstruct &omp) {
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 592c77289f9f29..5f4138e0f63e73 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -93,8 +93,7 @@ void DataSharingProcessor::insertDeallocs() {
       fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
       mlir::omp::PrivateClauseOp privatizer = symToPrivatizer.at(sym);
 
-      symTable->pushScope();
-
+      lower::SymMapScope scope(*symTable);
       mlir::OpBuilder::InsertionGuard guard(firOpBuilder);
 
       mlir::Region &deallocRegion = privatizer.getDeallocRegion();
@@ -108,8 +107,6 @@ void DataSharingProcessor::insertDeallocs() {
 
       converter.createHostAssociateVarCloneDealloc(*sym);
       firOpBuilder.create<mlir::omp::YieldOp>(hsb.getAddr().getLoc());
-
-      symTable->popScope();
     }
 }
 
@@ -488,8 +485,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
         isFirstPrivate ? mlir::omp::DataSharingClauseType::FirstPrivate
                        : mlir::omp::DataSharingClauseType::Private);
     fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
-
-    symTable->pushScope();
+    lower::SymMapScope outerScope(*symTable);
 
     // Populate the `alloc` region.
     {
@@ -507,7 +503,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
               .first;
 
       symTable->addSymbol(*sym, localExV);
-      symTable->pushScope();
+      lower::SymMapScope innerScope(*symTable);
       cloneSymbol(sym);
       mlir::Value cloneAddr = symTable->shallowLookupSymbol(*sym).getAddr();
       mlir::Type cloneType = cloneAddr.getType();
@@ -523,7 +519,6 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
 
       firOpBuilder.create<mlir::omp::YieldOp>(hsb.getAddr().getLoc(),
                                               yieldedValue);
-      symTable->popScope();
     }
 
     // Populate the `copy` region if this is a `firstprivate`.
@@ -548,7 +543,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
       };
 
       addSymbol(0, true);
-      symTable->pushScope();
+      lower::SymMapScope innerScope(*symTable);
       addSymbol(1);
 
       auto ip = firOpBuilder.saveInsertionPoint();
@@ -557,10 +552,8 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
       firOpBuilder.create<mlir::omp::YieldOp>(
           hsb.getAddr().getLoc(),
           symTable->shallowLookupSymbol(*sym).getAddr());
-      symTable->popScope();
     }
 
-    symTable->popScope();
     return result;
   }();
 
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 8b77f1ac6b4ff2..233aacb40354d4 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1546,7 +1546,7 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
   auto &builder = converter.getFirOpBuilder();
 
   // Insert privatizations before SECTIONS
-  symTable.pushScope();
+  lower::SymMapScope scope(symTable);
   DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
                            lower::omp::isLastItemInQueue(item, queue));
   dsp.processStep1();
@@ -1643,7 +1643,6 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
   if (clauseOps.nowait && !lastprivates.empty())
     builder.create<mlir::omp::BarrierOp>(loc);
 
-  symTable.popScope();
   return sectionsOp;
 }
 
@@ -2900,9 +2899,8 @@ void Fortran::lower::genOpenMPConstruct(lower::AbstractConverter &converter,
                                         semantics::SemanticsContext &semaCtx,
                                         lower::pft::Evaluation &eval,
                                         const parser::OpenMPConstruct &omp) {
-  symTable.pushScope();
+  lower::SymMapScope scope(symTable);
   genOMP(converter, symTable, semaCtx, eval, omp);
-  symTable.popScope();
 }
 
 void Fortran::lower::genOpenMPDeclarativeConstruct(

[jsjodin]

LGTM

[tblah]

LGTM, thanks!