From 433ca3ebbef50002bec716ef2c6d6a82db71048d Mon Sep 17 00:00:00 2001 From: Sergio Afonso Date: Tue, 10 Sep 2024 11:09:25 +0100 Subject: [PATCH] [Flang][Lower] Introduce SymMapScope helper class (NFC) (#107866) 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. --- flang/include/flang/Lower/SymbolMap.h | 10 ++++++++++ flang/lib/Lower/Bridge.cpp | 9 +++------ flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 15 ++++----------- flang/lib/Lower/OpenMP/OpenMP.cpp | 6 ++---- 4 files changed, 19 insertions(+), 21 deletions(-) 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> impliedDoStack; }; +/// RAII wrapper for SymMap. +class SymMapScope { +public: + explicit SymMapScope(SymMap &map) : map(map) { map.pushScope(); } + ~SymMapScope() { map.popScope(); } + +private: + SymMap ↦ +}; + } // 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>(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(kernel.t); @@ -3015,7 +3013,6 @@ class FirConverter : public Fortran::lower::AbstractConverter { builder->create(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(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(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( 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(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(