[flang] always run PolymorphicOpConversion sequentially (#90721)

It was pointed out in post commit review of
https://github.com/llvm/llvm-project/pull/90597 that the pass should
never have been run in parallel over all functions (and now other top
level operations) in the first place. The mutex used in the pass was
ineffective at preventing races since each instance of the pass would
have a different mutex.

7 files changed, 15 insertions, 51 deletions

diff --git a/flang/include/flang/Optimizer/Transforms/Passes.td b/flang/include/flang/Optimizer/Transforms/Passes.td
index dcb7037..1eaaa32 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.td
+++ b/flang/include/flang/Optimizer/Transforms/Passes.td
@@ -298,7 +298,7 @@ def AlgebraicSimplification : Pass<"flang-algebraic-simplification"> {
   let constructor = "::fir::createAlgebraicSimplificationPass()";
 }
 
-def PolymorphicOpConversion : Pass<"fir-polymorphic-op"> {
+def PolymorphicOpConversion : Pass<"fir-polymorphic-op", "mlir::ModuleOp"> {
   let summary =
     "Simplify operations on polymorphic types";
   let description = [{
diff --git a/flang/include/flang/Tools/CLOptions.inc b/flang/include/flang/Tools/CLOptions.inc
index bd60c66..d854364 100644
--- a/flang/include/flang/Tools/CLOptions.inc
+++ b/flang/include/flang/Tools/CLOptions.inc
@@ -271,7 +271,7 @@ inline void createDefaultFIROptimizerPassPipeline(
   pm.addPass(mlir::createCSEPass());
 
   // Polymorphic types
-  addNestedPassToAllTopLevelOperations(pm, fir::createPolymorphicOpConversion);
+  pm.addPass(fir::createPolymorphicOpConversion());
 
   if (pc.AliasAnalysis && !disableFirAliasTags && !useOldAliasTags)
     pm.addPass(fir::createAliasTagsPass());
diff --git a/flang/lib/Optimizer/Transforms/PolymorphicOpConversion.cpp b/flang/lib/Optimizer/Transforms/PolymorphicOpConversion.cpp
index 0f5c438..76c12d2 100644
--- a/flang/lib/Optimizer/Transforms/PolymorphicOpConversion.cpp
+++ b/flang/lib/Optimizer/Transforms/PolymorphicOpConversion.cpp
@@ -29,7 +29,6 @@
 #include "mlir/Transforms/DialectConversion.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/CommandLine.h"
-#include <mutex>
 
 namespace fir {
 #define GEN_PASS_DEF_POLYMORPHICOPCONVERSION
@@ -48,9 +47,8 @@ class SelectTypeConv : public OpConversionPattern<fir::SelectTypeOp> {
 public:
   using OpConversionPattern<fir::SelectTypeOp>::OpConversionPattern;
 
-  SelectTypeConv(mlir::MLIRContext *ctx, std::mutex *moduleMutex)
-      : mlir::OpConversionPattern<fir::SelectTypeOp>(ctx),
-        moduleMutex(moduleMutex) {}
+  SelectTypeConv(mlir::MLIRContext *ctx)
+      : mlir::OpConversionPattern<fir::SelectTypeOp>(ctx) {}
 
   mlir::LogicalResult
   matchAndRewrite(fir::SelectTypeOp selectType, OpAdaptor adaptor,
@@ -72,9 +70,6 @@ private:
 
   llvm::SmallSet<llvm::StringRef, 4> collectAncestors(fir::TypeInfoOp dt,
                                                       mlir::ModuleOp mod) const;
-
-  // Mutex used to guard insertion of mlir::func::FuncOp in the module.
-  std::mutex *moduleMutex;
 };
 
 /// Lower `fir.dispatch` operation. A virtual call to a method in a dispatch
@@ -223,21 +218,18 @@ class PolymorphicOpConversion
     : public fir::impl::PolymorphicOpConversionBase<PolymorphicOpConversion> {
 public:
   mlir::LogicalResult initialize(mlir::MLIRContext *ctx) override {
-    moduleMutex = new std::mutex();
     return mlir::success();
   }
 
   void runOnOperation() override {
     auto *context = &getContext();
-    auto mod = mlir::dyn_cast_or_null<mlir::ModuleOp>(getOperation());
-    if (!mod)
-      mod = getOperation()->getParentOfType<ModuleOp>();
+    mlir::ModuleOp mod = getOperation();
     mlir::RewritePatternSet patterns(context);
 
     BindingTables bindingTables;
     buildBindingTables(bindingTables, mod);
 
-    patterns.insert<SelectTypeConv>(context, moduleMutex);
+    patterns.insert<SelectTypeConv>(context);
     patterns.insert<DispatchOpConv>(context, bindingTables);
     mlir::ConversionTarget target(*context);
     target.addLegalDialect<mlir::affine::AffineDialect,
@@ -255,9 +247,6 @@ public:
       signalPassFailure();
     }
   }
-
-private:
-  std::mutex *moduleMutex;
 };
 } // namespace
 
@@ -410,7 +399,6 @@ mlir::LogicalResult SelectTypeConv::genTypeLadderStep(
     {
       // Since conversion is done in parallel for each fir.select_type
       // operation, the runtime function insertion must be threadsafe.
-      std::lock_guard<std::mutex> lock(*moduleMutex);
       callee =
           fir::createFuncOp(rewriter.getUnknownLoc(), mod, fctName,
                             rewriter.getFunctionType({descNoneTy, typeDescTy},
diff --git a/flang/test/Driver/bbc-mlir-pass-pipeline.f90 b/flang/test/Driver/bbc-mlir-pass-pipeline.f90
index 07b68bf..2cc25b3 100644
--- a/flang/test/Driver/bbc-mlir-pass-pipeline.f90
+++ b/flang/test/Driver/bbc-mlir-pass-pipeline.f90
@@ -45,18 +45,16 @@ end program
 ! CHECK-NEXT:   (S) 0 num-cse'd - Number of operations CSE'd
 ! CHECK-NEXT:   (S) 0 num-dce'd - Number of operations DCE'd
 
+! CHECK-NEXT: PolymorphicOpConversion
+
 ! CHECK-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
 ! CHECK-NEXT: 'fir.global' Pipeline
-! CHECK-NEXT:   PolymorphicOpConversion
 ! CHECK-NEXT:   CFGConversion
 ! CHECK-NEXT: 'func.func' Pipeline
-! CHECK-NEXT:   PolymorphicOpConversion
 ! CHECK-NEXT:   CFGConversion
 ! CHECK-NEXT: 'omp.declare_reduction' Pipeline
-! CHECK-NEXT:   PolymorphicOpConversion
 ! CHECK-NEXT:   CFGConversion
 ! CHECK-NEXT: 'omp.private' Pipeline
-! CHECK-NEXT:   PolymorphicOpConversion
 ! CHECK-NEXT:   CFGConversion
 
 ! CHECK-NEXT: SCFToControlFlow
diff --git a/flang/test/Driver/mlir-debug-pass-pipeline.f90 b/flang/test/Driver/mlir-debug-pass-pipeline.f90
index cad7415..a9980e3 100644
--- a/flang/test/Driver/mlir-debug-pass-pipeline.f90
+++ b/flang/test/Driver/mlir-debug-pass-pipeline.f90
@@ -65,18 +65,16 @@ end program
 ! ALL-NEXT:   (S) 0 num-cse'd - Number of operations CSE'd
 ! ALL-NEXT:   (S) 0 num-dce'd - Number of operations DCE'd
 
+! ALL-NEXT: PolymorphicOpConversion
+
 ! ALL-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
 ! ALL-NEXT:   'fir.global' Pipeline
-! ALL-NEXT:     PolymorphicOpConversion
 ! ALL-NEXT:     CFGConversion
 ! ALL-NEXT:   'func.func' Pipeline
-! ALL-NEXT:     PolymorphicOpConversion
 ! ALL-NEXT:     CFGConversion
 ! ALL-NEXT:   'omp.declare_reduction' Pipeline
-! ALL-NEXT:     PolymorphicOpConversion
 ! ALL-NEXT:     CFGConversion
 ! ALL-NEXT:   'omp.private' Pipeline
-! ALL-NEXT:     PolymorphicOpConversion
 ! ALL-NEXT:     CFGConversion
 ! ALL-NEXT: SCFToControlFlow
 ! ALL-NEXT: Canonicalizer
diff --git a/flang/test/Driver/mlir-pass-pipeline.f90 b/flang/test/Driver/mlir-pass-pipeline.f90
index 7f63f94..4ebac7c 100644
--- a/flang/test/Driver/mlir-pass-pipeline.f90
+++ b/flang/test/Driver/mlir-pass-pipeline.f90
@@ -1,8 +1,8 @@
 ! Test the MLIR pass pipeline
 
-! RUN: %flang_fc1 -S -mmlir --mlir-pass-statistics -mmlir --mlir-pass-statistics-display=pipeline -o /dev/null %s 2>&1 | FileCheck --check-prefixes=ALL,NOTO2 %s
+! RUN: %flang_fc1 -S -mmlir --mlir-pass-statistics -mmlir --mlir-pass-statistics-display=pipeline -o /dev/null %s 2>&1 | FileCheck --check-prefixes=ALL %s
 ! -O0 is the default:
-! RUN: %flang_fc1 -S -mmlir --mlir-pass-statistics -mmlir --mlir-pass-statistics-display=pipeline %s -O0 -o /dev/null 2>&1 | FileCheck --check-prefixes=ALL,NOTO2 %s
+! RUN: %flang_fc1 -S -mmlir --mlir-pass-statistics -mmlir --mlir-pass-statistics-display=pipeline %s -O0 -o /dev/null 2>&1 | FileCheck --check-prefixes=ALL %s
 ! RUN: %flang_fc1 -S -mmlir --mlir-pass-statistics -mmlir --mlir-pass-statistics-display=pipeline %s -O2 -o /dev/null 2>&1 | FileCheck --check-prefixes=ALL,O2 %s
 
 ! REQUIRES: asserts
@@ -56,28 +56,17 @@ end program
 ! ALL-NEXT:   (S) 0 num-cse'd - Number of operations CSE'd
 ! ALL-NEXT:   (S) 0 num-dce'd - Number of operations DCE'd
 
-! O2-NEXT:  Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
-! O2-NEXT:    'fir.global' Pipeline
-! O2-NEXT:      PolymorphicOpConversion
-! O2-NEXT:    'func.func' Pipeline
-! O2-NEXT:      PolymorphicOpConversion
-! O2-NEXT:    'omp.declare_reduction' Pipeline
-! O2-NEXT:      PolymorphicOpConversion
-! O2-NEXT:    'omp.private' Pipeline
-! O2-NEXT:      PolymorphicOpConversion
+! ALL-NEXT: PolymorphicOpConversion
 ! O2-NEXT:  AddAliasTags
+
 ! ALL-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
 ! ALL-NEXT:    'fir.global' Pipeline
-! NOTO2-NEXT:      PolymorphicOpConversion
 ! ALL-NEXT:      CFGConversion
 ! ALL-NEXT:    'func.func' Pipeline
-! NOTO2-NEXT:      PolymorphicOpConversion
 ! ALL-NEXT:      CFGConversion
 ! ALL-NEXT:   'omp.declare_reduction' Pipeline
-! NOTO2-NEXT:      PolymorphicOpConversion
 ! ALL-NEXT:      CFGConversion
 ! ALL-NEXT:   'omp.private' Pipeline
-! NOTO2-NEXT:    PolymorphicOpConversion
 ! ALL-NEXT:      CFGConversion
 
 ! ALL-NEXT: SCFToControlFlow
diff --git a/flang/test/Fir/basic-program.fir b/flang/test/Fir/basic-program.fir
index 67a9c56..02fb84e 100644
--- a/flang/test/Fir/basic-program.fir
+++ b/flang/test/Fir/basic-program.fir
@@ -62,16 +62,7 @@ func.func @_QQmain() {
 // PASSES-NEXT:   (S) 0 num-cse'd - Number of operations CSE'd
 // PASSES-NEXT:   (S) 0 num-dce'd - Number of operations DCE'd
 
-// PASSES-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
-// PASSES-NEXT: 'fir.global' Pipeline
-// PASSES-NEXT:   PolymorphicOpConversion
-// PASSES-NEXT: 'func.func' Pipeline
-// PASSES-NEXT:   PolymorphicOpConversion
-// PASSES-NEXT: 'omp.declare_reduction' Pipeline
-// PASSES-NEXT:   PolymorphicOpConversion
-// PASSES-NEXT: 'omp.private' Pipeline
-// PASSES-NEXT:   PolymorphicOpConversion
-
+// PASSES-NEXT: PolymorphicOpConversion
 // PASSES-NEXT: AddAliasTags
 
 // PASSES-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']