Skip to content

Commit d1b3648

Browse files
tblahtblah
authored
[flang] always run PolymorphicOpConversion sequentially (#90721)
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.
1 parent 28869a7 commit d1b3648

File tree

7 files changed

+15
-51
lines changed

7 files changed

+15
-51
lines changed

‎flang/include/flang/Optimizer/Transforms/Passes.td

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ def AlgebraicSimplification : Pass<"flang-algebraic-simplification"> {
298298
let constructor = "::fir::createAlgebraicSimplificationPass()";
299299
}
300300

301-
def PolymorphicOpConversion : Pass<"fir-polymorphic-op"> {
301+
def PolymorphicOpConversion : Pass<"fir-polymorphic-op", "mlir::ModuleOp"> {
302302
let summary =
303303
"Simplify operations on polymorphic types";
304304
let description = [{

‎flang/include/flang/Tools/CLOptions.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ inline void createDefaultFIROptimizerPassPipeline(
271271
pm.addPass(mlir::createCSEPass());
272272

273273
// Polymorphic types
274-
addNestedPassToAllTopLevelOperations(pm, fir::createPolymorphicOpConversion);
274+
pm.addPass(fir::createPolymorphicOpConversion());
275275

276276
if (pc.AliasAnalysis && !disableFirAliasTags && !useOldAliasTags)
277277
pm.addPass(fir::createAliasTagsPass());

‎flang/lib/Optimizer/Transforms/PolymorphicOpConversion.cpp

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
#include "mlir/Transforms/DialectConversion.h"
3030
#include "llvm/ADT/SmallSet.h"
3131
#include "llvm/Support/CommandLine.h"
32-
#include <mutex>
3332

3433
namespace fir {
3534
#define GEN_PASS_DEF_POLYMORPHICOPCONVERSION
@@ -48,9 +47,8 @@ class SelectTypeConv : public OpConversionPattern<fir::SelectTypeOp> {
4847
public:
4948
using OpConversionPattern<fir::SelectTypeOp>::OpConversionPattern;
5049

51-
SelectTypeConv(mlir::MLIRContext *ctx, std::mutex *moduleMutex)
52-
: mlir::OpConversionPattern<fir::SelectTypeOp>(ctx),
53-
moduleMutex(moduleMutex) {}
50+
SelectTypeConv(mlir::MLIRContext *ctx)
51+
: mlir::OpConversionPattern<fir::SelectTypeOp>(ctx) {}
5452

5553
mlir::LogicalResult
5654
matchAndRewrite(fir::SelectTypeOp selectType, OpAdaptor adaptor,
@@ -72,9 +70,6 @@ class SelectTypeConv : public OpConversionPattern<fir::SelectTypeOp> {
7270

7371
llvm::SmallSet<llvm::StringRef, 4> collectAncestors(fir::TypeInfoOp dt,
7472
mlir::ModuleOp mod) const;
75-
76-
// Mutex used to guard insertion of mlir::func::FuncOp in the module.
77-
std::mutex *moduleMutex;
7873
};
7974

8075
/// Lower `fir.dispatch` operation. A virtual call to a method in a dispatch
@@ -223,21 +218,18 @@ class PolymorphicOpConversion
223218
: public fir::impl::PolymorphicOpConversionBase<PolymorphicOpConversion> {
224219
public:
225220
mlir::LogicalResult initialize(mlir::MLIRContext *ctx) override {
226-
moduleMutex = new std::mutex();
227221
return mlir::success();
228222
}
229223

230224
void runOnOperation() override {
231225
auto *context = &getContext();
232-
auto mod = mlir::dyn_cast_or_null<mlir::ModuleOp>(getOperation());
233-
if (!mod)
234-
mod = getOperation()->getParentOfType<ModuleOp>();
226+
mlir::ModuleOp mod = getOperation();
235227
mlir::RewritePatternSet patterns(context);
236228

237229
BindingTables bindingTables;
238230
buildBindingTables(bindingTables, mod);
239231

240-
patterns.insert<SelectTypeConv>(context, moduleMutex);
232+
patterns.insert<SelectTypeConv>(context);
241233
patterns.insert<DispatchOpConv>(context, bindingTables);
242234
mlir::ConversionTarget target(*context);
243235
target.addLegalDialect<mlir::affine::AffineDialect,
@@ -255,9 +247,6 @@ class PolymorphicOpConversion
255247
signalPassFailure();
256248
}
257249
}
258-
259-
private:
260-
std::mutex *moduleMutex;
261250
};
262251
} // namespace
263252

@@ -410,7 +399,6 @@ mlir::LogicalResult SelectTypeConv::genTypeLadderStep(
410399
{
411400
// Since conversion is done in parallel for each fir.select_type
412401
// operation, the runtime function insertion must be threadsafe.
413-
std::lock_guard<std::mutex> lock(*moduleMutex);
414402
callee =
415403
fir::createFuncOp(rewriter.getUnknownLoc(), mod, fctName,
416404
rewriter.getFunctionType({descNoneTy, typeDescTy},

‎flang/test/Driver/bbc-mlir-pass-pipeline.f90

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,18 +45,16 @@
4545
! CHECK-NEXT: (S) 0 num-cse'd - Number of operations CSE'd
4646
! CHECK-NEXT: (S) 0 num-dce'd - Number of operations DCE'd
4747

48+
! CHECK-NEXT: PolymorphicOpConversion
49+
4850
! CHECK-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
4951
! CHECK-NEXT: 'fir.global' Pipeline
50-
! CHECK-NEXT: PolymorphicOpConversion
5152
! CHECK-NEXT: CFGConversion
5253
! CHECK-NEXT: 'func.func' Pipeline
53-
! CHECK-NEXT: PolymorphicOpConversion
5454
! CHECK-NEXT: CFGConversion
5555
! CHECK-NEXT: 'omp.declare_reduction' Pipeline
56-
! CHECK-NEXT: PolymorphicOpConversion
5756
! CHECK-NEXT: CFGConversion
5857
! CHECK-NEXT: 'omp.private' Pipeline
59-
! CHECK-NEXT: PolymorphicOpConversion
6058
! CHECK-NEXT: CFGConversion
6159

6260
! CHECK-NEXT: SCFToControlFlow

‎flang/test/Driver/mlir-debug-pass-pipeline.f90

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,18 +65,16 @@
6565
! ALL-NEXT: (S) 0 num-cse'd - Number of operations CSE'd
6666
! ALL-NEXT: (S) 0 num-dce'd - Number of operations DCE'd
6767

68+
! ALL-NEXT: PolymorphicOpConversion
69+
6870
! ALL-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
6971
! ALL-NEXT: 'fir.global' Pipeline
70-
! ALL-NEXT: PolymorphicOpConversion
7172
! ALL-NEXT: CFGConversion
7273
! ALL-NEXT: 'func.func' Pipeline
73-
! ALL-NEXT: PolymorphicOpConversion
7474
! ALL-NEXT: CFGConversion
7575
! ALL-NEXT: 'omp.declare_reduction' Pipeline
76-
! ALL-NEXT: PolymorphicOpConversion
7776
! ALL-NEXT: CFGConversion
7877
! ALL-NEXT: 'omp.private' Pipeline
79-
! ALL-NEXT: PolymorphicOpConversion
8078
! ALL-NEXT: CFGConversion
8179
! ALL-NEXT: SCFToControlFlow
8280
! ALL-NEXT: Canonicalizer

‎flang/test/Driver/mlir-pass-pipeline.f90

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
! Test the MLIR pass pipeline
22

3-
! 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
3+
! 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
44
! -O0 is the default:
5-
! 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
5+
! 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
66
! 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
77

88
! REQUIRES: asserts
@@ -56,28 +56,17 @@
5656
! ALL-NEXT: (S) 0 num-cse'd - Number of operations CSE'd
5757
! ALL-NEXT: (S) 0 num-dce'd - Number of operations DCE'd
5858

59-
! O2-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
60-
! O2-NEXT: 'fir.global' Pipeline
61-
! O2-NEXT: PolymorphicOpConversion
62-
! O2-NEXT: 'func.func' Pipeline
63-
! O2-NEXT: PolymorphicOpConversion
64-
! O2-NEXT: 'omp.declare_reduction' Pipeline
65-
! O2-NEXT: PolymorphicOpConversion
66-
! O2-NEXT: 'omp.private' Pipeline
67-
! O2-NEXT: PolymorphicOpConversion
59+
! ALL-NEXT: PolymorphicOpConversion
6860
! O2-NEXT: AddAliasTags
61+
6962
! ALL-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
7063
! ALL-NEXT: 'fir.global' Pipeline
71-
! NOTO2-NEXT: PolymorphicOpConversion
7264
! ALL-NEXT: CFGConversion
7365
! ALL-NEXT: 'func.func' Pipeline
74-
! NOTO2-NEXT: PolymorphicOpConversion
7566
! ALL-NEXT: CFGConversion
7667
! ALL-NEXT: 'omp.declare_reduction' Pipeline
77-
! NOTO2-NEXT: PolymorphicOpConversion
7868
! ALL-NEXT: CFGConversion
7969
! ALL-NEXT: 'omp.private' Pipeline
80-
! NOTO2-NEXT: PolymorphicOpConversion
8170
! ALL-NEXT: CFGConversion
8271

8372
! ALL-NEXT: SCFToControlFlow

‎flang/test/Fir/basic-program.fir

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,16 +62,7 @@ func.func @_QQmain() {
6262
// PASSES-NEXT: (S) 0 num-cse'd - Number of operations CSE'd
6363
// PASSES-NEXT: (S) 0 num-dce'd - Number of operations DCE'd
6464

65-
// PASSES-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']
66-
// PASSES-NEXT: 'fir.global' Pipeline
67-
// PASSES-NEXT: PolymorphicOpConversion
68-
// PASSES-NEXT: 'func.func' Pipeline
69-
// PASSES-NEXT: PolymorphicOpConversion
70-
// PASSES-NEXT: 'omp.declare_reduction' Pipeline
71-
// PASSES-NEXT: PolymorphicOpConversion
72-
// PASSES-NEXT: 'omp.private' Pipeline
73-
// PASSES-NEXT: PolymorphicOpConversion
74-
65+
// PASSES-NEXT: PolymorphicOpConversion
7566
// PASSES-NEXT: AddAliasTags
7667

7768
// PASSES-NEXT: Pipeline Collection : ['fir.global', 'func.func', 'omp.declare_reduction', 'omp.private']

0 commit comments

Comments
 (0)