[flang] always run PolymorphicOpConversion sequentially #90721
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was pointed out in post commit review of llvm#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.
llvmbot
added
flang:driver
flang
|
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-flang-fir-hlfir Author: Tom Eccles (tblah) ChangesIt was pointed out in post commit review of #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. Full diff: https://github.com/llvm/llvm-project/pull/90721.diff 7 Files Affected:
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.td b/flang/include/flang/Optimizer/Transforms/Passes.td
index dcb7037e2991be..1eaaa32a508a03 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 bd60c66b61ad24..d8543648987036 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 0f5c43882ee30a..76c12d2de5c444 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 @@ class SelectTypeConv : public OpConversionPattern<fir::SelectTypeOp> {
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 @@ class PolymorphicOpConversion
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 07b68bfe03b336..2cc25b3c473feb 100644
--- a/flang/test/Driver/bbc-mlir-pass-pipeline.f90
+++ b/flang/test/Driver/bbc-mlir-pass-pipeline.f90
@@ -45,18 +45,16 @@
! 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 cad7415a3b528d..a9980e3c932c81 100644
--- a/flang/test/Driver/mlir-debug-pass-pipeline.f90
+++ b/flang/test/Driver/mlir-debug-pass-pipeline.f90
@@ -65,18 +65,16 @@
! 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 7f63f946c2fbd9..4ebac7c3fb65c1 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 @@
! 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 67a9c56ed9acb5..02fb84ed8c873d 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']
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was pointed out in post commit review of #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.