[Flang][OpenMP] Simplify privatization (#150)

This patch removes some redundant information from the `DataSharingProcessor` class that was initially introduced to support wrapper-based representation of loop constructs and that is no longer needed. Usage of this class is also simplified as a result.
ROCm · Aug 29, 2024 · a723fb3 · a723fb3
1 parent e98cfd7
commit a723fb3
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 46 deletions.
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -56,25 +56,26 @@ void DataSharingProcessor::processStep1() {
   collectPreDeterminedSymbols();
 }
 
-void DataSharingProcessor::processStep2() {
+void DataSharingProcessor::processStep2(
+    mlir::omp::PrivateClauseOps *clauseOps) {
   if (privatizationDone)
     return;
 
-  privatize();
+  privatize(clauseOps);
   insertBarrier();
   privatizationDone = true;
 }
 
 void DataSharingProcessor::processStep3(mlir::Operation *op, bool isLoop) {
-  //  'sections' lastprivate is handled by genOMP()
+  // 'sections' lastprivate is handled by genOMP()
   if (!mlir::isa<mlir::omp::SectionsOp>(op)) {
     insPt = firOpBuilder.saveInsertionPoint();
     copyLastPrivatize(op);
     firOpBuilder.restoreInsertionPoint(insPt);
   }
 
   if (isLoop) {
-    //  push deallocs out of the loop
+    // push deallocs out of the loop
     firOpBuilder.setInsertionPointAfter(op);
     insertDeallocs();
   } else {
@@ -479,14 +480,14 @@ void DataSharingProcessor::collectPreDeterminedSymbols() {
                    preDeterminedSymbols);
 }
 
-void DataSharingProcessor::privatize() {
+void DataSharingProcessor::privatize(mlir::omp::PrivateClauseOps *clauseOps) {
   for (const semantics::Symbol *sym : allPrivatizedSymbols) {
     if (const auto *commonDet =
             sym->detailsIf<semantics::CommonBlockDetails>()) {
       for (const auto &mem : commonDet->objects())
-        doPrivatize(&*mem);
+        doPrivatize(&*mem, clauseOps);
     } else {
-      doPrivatize(sym);
+      doPrivatize(sym, clauseOps);
     }
   }
 }
@@ -504,7 +505,8 @@ void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
     }
 }
 
-void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym) {
+void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
+                                       mlir::omp::PrivateClauseOps *clauseOps) {
   if (!useDelayedPrivatization) {
     cloneSymbol(sym);
     copyFirstPrivateSymbol(sym);
@@ -615,11 +617,10 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym) {
     return result;
   }();
 
-  privateClauseOps.privateSyms.push_back(
-      mlir::SymbolRefAttr::get(privatizerOp));
-  privateClauseOps.privateVars.push_back(hsb.getAddr());
-  delayedPrivSyms.push_back(sym);
-
+  if (clauseOps) {
+    clauseOps->privateSyms.push_back(mlir::SymbolRefAttr::get(privatizerOp));
+    clauseOps->privateVars.push_back(hsb.getAddr());
+  }
   symToPrivatizer[sym] = privatizerOp;
 }
 

diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -91,8 +91,6 @@ class DataSharingProcessor {
   lower::SymMap *symTable;
 
   OMPConstructSymbolVisitor visitor;
-  mlir::omp::PrivateClauseOps privateClauseOps;
-  llvm::SmallVector<const semantics::Symbol *> delayedPrivSyms;
   bool privatizationDone = false;
 
   bool needBarrier();
@@ -109,10 +107,9 @@ class DataSharingProcessor {
   void collectDefaultSymbols();
   void collectImplicitSymbols();
   void collectPreDeterminedSymbols();
-  void privatize();
-  void defaultPrivatize();
-  void implicitPrivatize();
-  void doPrivatize(const semantics::Symbol *sym);
+  void privatize(mlir::omp::PrivateClauseOps *clauseOps);
+  void doPrivatize(const semantics::Symbol *sym,
+                   mlir::omp::PrivateClauseOps *clauseOps);
   void copyLastPrivatize(mlir::Operation *op);
   void insertLastPrivateCompare(mlir::Operation *op);
   void cloneSymbol(const semantics::Symbol *sym);
@@ -157,7 +154,7 @@ class DataSharingProcessor {
   // before the operation is created since the bounds of the MLIR OpenMP
   // operation can be privatised.
   void processStep1();
-  void processStep2();
+  void processStep2(mlir::omp::PrivateClauseOps *clauseOps = nullptr);
   void processStep3(mlir::Operation *op, bool isLoop);
 
   void pushLoopIV(mlir::Value iv) { loopIVs.push_back(iv); }
@@ -167,12 +164,10 @@ class DataSharingProcessor {
     return allPrivatizedSymbols;
   }
 
-  const mlir::omp::PrivateClauseOps &getPrivateClauseOps() const {
-    return privateClauseOps;
-  }
-
   llvm::ArrayRef<const semantics::Symbol *> getDelayedPrivSyms() const {
-    return delayedPrivSyms;
+    return useDelayedPrivatization
+               ? allPrivatizedSymbols.getArrayRef()
+               : llvm::ArrayRef<const semantics::Symbol *>();
   }
 };
 

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1946,11 +1946,7 @@ genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
                            lower::omp::isLastItemInQueue(item, queue),
                            /*useDelayedPrivatization=*/true, &symTable);
   dsp.processStep1();
-  dsp.processStep2();
-
-  const auto &privateClauseOps = dsp.getPrivateClauseOps();
-  clauseOps.privateVars = privateClauseOps.privateVars;
-  clauseOps.privateSyms = privateClauseOps.privateSyms;
+  dsp.processStep2(&clauseOps);
 
   auto genRegionEntryCB = [&](mlir::Operation *op) {
     auto parallelOp = llvm::cast<mlir::omp::ParallelOp>(op);
@@ -2026,12 +2022,6 @@ static mlir::omp::ParallelOp genParallelCompositeOp(
     DataSharingProcessor &dsp) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
 
-  if (enableDelayedPrivatization) {
-    const auto &privateClauseOps = dsp.getPrivateClauseOps();
-    clauseOps.privateVars = privateClauseOps.privateVars;
-    clauseOps.privateSyms = privateClauseOps.privateSyms;
-  }
-
   // Create omp.parallel operation.
   auto parallelOp = firOpBuilder.create<mlir::omp::ParallelOp>(loc, clauseOps);
   parallelOp.setComposite(/*val=*/true);
@@ -2268,14 +2258,8 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
                            lower::omp::isLastItemInQueue(item, queue),
                            enableDelayedPrivatizationStaging, &symTable);
   dsp.processStep1();
-
-  if (enableDelayedPrivatizationStaging) {
-    dsp.processStep2();
-
-    const auto &privateClauseOps = dsp.getPrivateClauseOps();
-    clauseOps.privateVars = privateClauseOps.privateVars;
-    clauseOps.privateSyms = privateClauseOps.privateSyms;
-  }
+  if (enableDelayedPrivatizationStaging)
+    dsp.processStep2(&clauseOps);
 
   // 5.8.1 Implicit Data-Mapping Attribute Rules
   // The following code follows the implicit data-mapping rules to map all the
@@ -2711,7 +2695,7 @@ static void genCompositeDistributeParallelDo(
                            /*shouldCollectPreDeterminedSymbols=*/true,
                            /*useDelayedPrivatization=*/true, &symTable);
   dsp.processStep1();
-  dsp.processStep2();
+  dsp.processStep2(enableDelayedPrivatization ? &parallelClauseOps : nullptr);
 
   genParallelCompositeOp(converter, semaCtx, parallelItem->clauses, eval, loc,
                          parallelClauseOps, numThreadsClauseOps,
@@ -2791,7 +2775,7 @@ static void genCompositeDistributeParallelDoSimd(
                            /*shouldCollectPreDeterminedSymbols=*/true,
                            /*useDelayedPrivatization=*/true, &symTable);
   dsp.processStep1();
-  dsp.processStep2();
+  dsp.processStep2(enableDelayedPrivatization ? &parallelClauseOps : nullptr);
 
   genParallelCompositeOp(converter, semaCtx, parallelItem->clauses, eval, loc,
                          parallelClauseOps, numThreadsClauseOps,