From e7790fbed32b729ad59cea4b77d152514605cb0e Mon Sep 17 00:00:00 2001 From: Matthias Springer Date: Wed, 4 Jan 2023 11:39:41 +0100 Subject: [PATCH] [mlir] Add `test-convergence` option to Canonicalizer tests This new option is set to `false` by default. It should be set only in Canonicalizer tests to detect faulty canonicalization patterns. I.e., patterns that prevent the canonicalizer from converging. The canonicalizer should always convergence on such small unit tests that we have in `canonicalize.mlir`. Two faulty canonicalization patterns were detected and fixed with this change. Differential Revision: https://reviews.llvm.org/D140873 --- mlir/include/mlir/Transforms/Passes.td | 4 +++- mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | 4 +++- mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp | 15 +++++++-------- mlir/lib/Transforms/Canonicalizer.cpp | 5 ++++- mlir/test/Dialect/AMDGPU/canonicalize.mlir | 2 +- mlir/test/Dialect/Affine/canonicalize.mlir | 4 ++-- mlir/test/Dialect/Arith/canonicalize.mlir | 2 +- mlir/test/Dialect/Bufferization/canonicalize.mlir | 5 +++-- mlir/test/Dialect/Builtin/canonicalize.mlir | 2 +- mlir/test/Dialect/Complex/canonicalize.mlir | 4 ++-- mlir/test/Dialect/ControlFlow/canonicalize.mlir | 2 +- mlir/test/Dialect/GPU/canonicalize.mlir | 2 +- mlir/test/Dialect/LLVMIR/canonicalize.mlir | 2 +- mlir/test/Dialect/Linalg/canonicalize.mlir | 2 +- mlir/test/Dialect/Math/canonicalize.mlir | 2 +- mlir/test/Dialect/MemRef/canonicalize.mlir | 2 +- mlir/test/Dialect/OpenACC/canonicalize.mlir | 2 +- mlir/test/Dialect/OpenMP/canonicalize.mlir | 2 +- mlir/test/Dialect/PDL/canonicalize.mlir | 2 +- mlir/test/Dialect/Quant/canonicalize.mlir | 2 +- mlir/test/Dialect/SCF/canonicalize.mlir | 5 +---- mlir/test/Dialect/SPIRV/Transforms/canonicalize.mlir | 2 +- mlir/test/Dialect/Shape/canonicalize.mlir | 2 +- mlir/test/Dialect/Tensor/canonicalize.mlir | 2 +- mlir/test/Dialect/Tosa/canonicalize.mlir | 2 +- mlir/test/Dialect/Vector/canonicalize.mlir | 4 +--- mlir/test/Pass/run-reproducer.mlir | 2 +- mlir/test/Transforms/canonicalize.mlir | 2 +- 28 files changed, 45 insertions(+), 43 deletions(-) diff --git a/mlir/include/mlir/Transforms/Passes.td b/mlir/include/mlir/Transforms/Passes.td index fd6351d..e2a1593 100644 --- a/mlir/include/mlir/Transforms/Passes.td +++ b/mlir/include/mlir/Transforms/Passes.td @@ -39,7 +39,9 @@ def Canonicalizer : Pass<"canonicalize"> { /*default=*/"10", "Max. iterations between applying patterns / simplifying regions">, Option<"maxNumRewrites", "max-num-rewrites", "int64_t", /*default=*/"-1", - "Max. number of pattern rewrites within an iteration"> + "Max. number of pattern rewrites within an iteration">, + Option<"testConvergence", "test-convergence", "bool", /*default=*/"false", + "Test only: Fail pass on non-convergence to detect cyclic pattern"> ] # RewritePassUtils.options; } diff --git a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp index d687043..8ef5483 100644 --- a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp +++ b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp @@ -687,6 +687,8 @@ struct FoldLaunchArguments : public OpRewritePattern { // Check if size is trivially one. if (!matchPattern(size, m_One())) return; + if (id.getUses().empty()) + return; if (!simplified) { // Create a zero value the first time. OpBuilder::InsertionGuard guard(rewriter); @@ -694,7 +696,7 @@ struct FoldLaunchArguments : public OpRewritePattern { zero = rewriter.create(op.getLoc(), /*value=*/0); } - id.replaceAllUsesWith(zero); + rewriter.replaceAllUsesWith(id, zero); simplified = true; }; constPropIdUses(op.getBlockIds().x, op.getGridSizeX()); diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp index 7a5de2f..fb9835a 100644 --- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp +++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp @@ -178,16 +178,15 @@ struct RemoveConstantIfCondition : public OpRewritePattern { // Early return if there is no condition. Value ifCond = op.getIfCond(); if (!ifCond) - return success(); + return failure(); IntegerAttr constAttr; - if (matchPattern(ifCond, m_Constant(&constAttr))) { - if (constAttr.getInt()) - rewriter.updateRootInPlace(op, - [&]() { op.getIfCondMutable().erase(0); }); - else - rewriter.eraseOp(op); - } + if (!matchPattern(ifCond, m_Constant(&constAttr))) + return failure(); + if (constAttr.getInt()) + rewriter.updateRootInPlace(op, [&]() { op.getIfCondMutable().erase(0); }); + else + rewriter.eraseOp(op); return success(); } diff --git a/mlir/lib/Transforms/Canonicalizer.cpp b/mlir/lib/Transforms/Canonicalizer.cpp index a6fa775..b4ad85c 100644 --- a/mlir/lib/Transforms/Canonicalizer.cpp +++ b/mlir/lib/Transforms/Canonicalizer.cpp @@ -57,8 +57,11 @@ struct Canonicalizer : public impl::CanonicalizerBase { config.enableRegionSimplification = enableRegionSimplification; config.maxIterations = maxIterations; config.maxNumRewrites = maxNumRewrites; + LogicalResult converged = + applyPatternsAndFoldGreedily(getOperation(), patterns, config); // Canonicalization is best-effort. Non-convergence is not a pass failure. - (void)applyPatternsAndFoldGreedily(getOperation(), patterns, config); + if (testConvergence && failed(converged)) + signalPassFailure(); } FrozenRewritePatternSet patterns; diff --git a/mlir/test/Dialect/AMDGPU/canonicalize.mlir b/mlir/test/Dialect/AMDGPU/canonicalize.mlir index d984f8b..4559e39 100644 --- a/mlir/test/Dialect/AMDGPU/canonicalize.mlir +++ b/mlir/test/Dialect/AMDGPU/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -split-input-file -canonicalize | FileCheck %s +// RUN: mlir-opt %s -split-input-file -canonicalize="test-convergence" | FileCheck %s // CHECK-LABEL: func @known_oob_load func.func @known_oob_load(%arg0: memref<4xf32>) -> f32 { diff --git a/mlir/test/Dialect/Affine/canonicalize.mlir b/mlir/test/Dialect/Affine/canonicalize.mlir index e47cdde..1dac401 100644 --- a/mlir/test/Dialect/Affine/canonicalize.mlir +++ b/mlir/test/Dialect/Affine/canonicalize.mlir @@ -1,5 +1,5 @@ -// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -canonicalize | FileCheck %s -// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -canonicalize="top-down=0" | FileCheck %s --check-prefix=CHECK-BOTTOM-UP +// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -canonicalize="test-convergence" | FileCheck %s +// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -canonicalize="test-convergence top-down=0" | FileCheck %s --check-prefix=CHECK-BOTTOM-UP // ----- diff --git a/mlir/test/Dialect/Arith/canonicalize.mlir b/mlir/test/Dialect/Arith/canonicalize.mlir index 02cbaa2..5806c9c 100644 --- a/mlir/test/Dialect/Arith/canonicalize.mlir +++ b/mlir/test/Dialect/Arith/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize --split-input-file | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" --split-input-file | FileCheck %s // CHECK-LABEL: @select_same_val // CHECK: return %arg1 diff --git a/mlir/test/Dialect/Bufferization/canonicalize.mlir b/mlir/test/Dialect/Bufferization/canonicalize.mlir index df34039..fae4351 100644 --- a/mlir/test/Dialect/Bufferization/canonicalize.mlir +++ b/mlir/test/Dialect/Bufferization/canonicalize.mlir @@ -1,5 +1,6 @@ -// RUN: mlir-opt %s -canonicalize --split-input-file \ -// RUN: -allow-unregistered-dialect |\ +// RUN: mlir-opt %s \ +// RUN: -canonicalize="test-convergence" \ +// RUN: --split-input-file -allow-unregistered-dialect | \ // RUN: FileCheck %s // Basic folding of to_tensor(to_memref(t)) -> t diff --git a/mlir/test/Dialect/Builtin/canonicalize.mlir b/mlir/test/Dialect/Builtin/canonicalize.mlir index 6e29429..2e36b7e 100644 --- a/mlir/test/Dialect/Builtin/canonicalize.mlir +++ b/mlir/test/Dialect/Builtin/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" | FileCheck %s //===----------------------------------------------------------------------===// // UnrealizedConversionCastOp diff --git a/mlir/test/Dialect/Complex/canonicalize.mlir b/mlir/test/Dialect/Complex/canonicalize.mlir index 1b85837..f0d287f 100644 --- a/mlir/test/Dialect/Complex/canonicalize.mlir +++ b/mlir/test/Dialect/Complex/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" | FileCheck %s // CHECK-LABEL: func @create_of_real_and_imag // CHECK-SAME: (%[[CPLX:.*]]: complex) @@ -154,4 +154,4 @@ func.func @complex_sub_zero() -> complex { // CHECK-NEXT: return %[[CPLX:.*]] : complex %sub = complex.sub %complex1, %complex2 : complex return %sub : complex -} \ No newline at end of file +} diff --git a/mlir/test/Dialect/ControlFlow/canonicalize.mlir b/mlir/test/Dialect/ControlFlow/canonicalize.mlir index 8cef845..0ad6898 100644 --- a/mlir/test/Dialect/ControlFlow/canonicalize.mlir +++ b/mlir/test/Dialect/ControlFlow/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -allow-unregistered-dialect -pass-pipeline='builtin.module(func.func(canonicalize))' -split-input-file | FileCheck --dump-input-context 20 %s +// RUN: mlir-opt %s -allow-unregistered-dialect -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' -split-input-file | FileCheck --dump-input-context 20 %s /// Test the folding of BranchOp. diff --git a/mlir/test/Dialect/GPU/canonicalize.mlir b/mlir/test/Dialect/GPU/canonicalize.mlir index eedc238..99633ff 100644 --- a/mlir/test/Dialect/GPU/canonicalize.mlir +++ b/mlir/test/Dialect/GPU/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize --split-input-file -allow-unregistered-dialect | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" --split-input-file -allow-unregistered-dialect | FileCheck %s // Fold all the gpu.wait ops as they are redundant. // CHECK-LABEL: func @fold_wait_op_test1 diff --git a/mlir/test/Dialect/LLVMIR/canonicalize.mlir b/mlir/test/Dialect/LLVMIR/canonicalize.mlir index 9432eda..9a3309d 100644 --- a/mlir/test/Dialect/LLVMIR/canonicalize.mlir +++ b/mlir/test/Dialect/LLVMIR/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt -canonicalize %s -split-input-file | FileCheck %s +// RUN: mlir-opt -canonicalize="test-convergence" %s -split-input-file | FileCheck %s // CHECK-LABEL: fold_extractvalue llvm.func @fold_extractvalue() -> i32 { diff --git a/mlir/test/Dialect/Linalg/canonicalize.mlir b/mlir/test/Dialect/Linalg/canonicalize.mlir index 4510d20..9e4d886c 100644 --- a/mlir/test/Dialect/Linalg/canonicalize.mlir +++ b/mlir/test/Dialect/Linalg/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize -split-input-file | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" -split-input-file | FileCheck %s // CHECK-LABEL: func @memref_cast( func.func @memref_cast(%a: index, %b: index) -> memref { diff --git a/mlir/test/Dialect/Math/canonicalize.mlir b/mlir/test/Dialect/Math/canonicalize.mlir index f3825cd..7a5194b 100644 --- a/mlir/test/Dialect/Math/canonicalize.mlir +++ b/mlir/test/Dialect/Math/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" | FileCheck %s // CHECK-LABEL: @ceil_fold // CHECK: %[[cst:.+]] = arith.constant 1.000000e+00 : f32 diff --git a/mlir/test/Dialect/MemRef/canonicalize.mlir b/mlir/test/Dialect/MemRef/canonicalize.mlir index 88d9155..3d9f71e 100644 --- a/mlir/test/Dialect/MemRef/canonicalize.mlir +++ b/mlir/test/Dialect/MemRef/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize --split-input-file -allow-unregistered-dialect | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" --split-input-file -allow-unregistered-dialect | FileCheck %s // CHECK-LABEL: func @subview_of_size_memcast // CHECK-SAME: %[[ARG0:.[a-z0-9A-Z_]+]]: memref<4x6x16x32xi8> diff --git a/mlir/test/Dialect/OpenACC/canonicalize.mlir b/mlir/test/Dialect/OpenACC/canonicalize.mlir index 71c388c..10cb19f 100644 --- a/mlir/test/Dialect/OpenACC/canonicalize.mlir +++ b/mlir/test/Dialect/OpenACC/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize -split-input-file | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" -split-input-file | FileCheck %s func.func @testenterdataop(%a: memref<10xf32>) -> () { %ifCond = arith.constant true diff --git a/mlir/test/Dialect/OpenMP/canonicalize.mlir b/mlir/test/Dialect/OpenMP/canonicalize.mlir index c5ab769..c5d18f3 100644 --- a/mlir/test/Dialect/OpenMP/canonicalize.mlir +++ b/mlir/test/Dialect/OpenMP/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -canonicalize -split-input-file | FileCheck %s +// RUN: mlir-opt %s -canonicalize="test-convergence" -split-input-file | FileCheck %s func.func @update_no_op(%x : memref) { omp.atomic.update %x : memref { diff --git a/mlir/test/Dialect/PDL/canonicalize.mlir b/mlir/test/Dialect/PDL/canonicalize.mlir index 94688a2..ee2a6f7 100644 --- a/mlir/test/Dialect/PDL/canonicalize.mlir +++ b/mlir/test/Dialect/PDL/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt -canonicalize %s | FileCheck %s +// RUN: mlir-opt -canonicalize="test-convergence" %s | FileCheck %s pdl.pattern @operation_op : benefit(1) { %root = operation "foo.op" diff --git a/mlir/test/Dialect/Quant/canonicalize.mlir b/mlir/test/Dialect/Quant/canonicalize.mlir index c67f129..36c3eaf 100644 --- a/mlir/test/Dialect/Quant/canonicalize.mlir +++ b/mlir/test/Dialect/Quant/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -split-input-file -pass-pipeline='builtin.module(func.func(canonicalize))' | FileCheck %s +// RUN: mlir-opt %s -split-input-file -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' | FileCheck %s // ----- // CHECK-LABEL: redundant_scast diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir index e5e2afc..220adc5 100644 --- a/mlir/test/Dialect/SCF/canonicalize.mlir +++ b/mlir/test/Dialect/SCF/canonicalize.mlir @@ -1,7 +1,4 @@ -// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize))' -split-input-file | FileCheck %s - - -// ----- +// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' -split-input-file | FileCheck %s func.func @single_iteration_some(%A: memref) { %c0 = arith.constant 0 : index diff --git a/mlir/test/Dialect/SPIRV/Transforms/canonicalize.mlir b/mlir/test/Dialect/SPIRV/Transforms/canonicalize.mlir index e65f92e..518ad2e 100644 --- a/mlir/test/Dialect/SPIRV/Transforms/canonicalize.mlir +++ b/mlir/test/Dialect/SPIRV/Transforms/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -split-input-file -pass-pipeline='builtin.module(func.func(canonicalize))' | FileCheck %s +// RUN: mlir-opt %s -split-input-file -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' | FileCheck %s //===----------------------------------------------------------------------===// // spirv.AccessChain diff --git a/mlir/test/Dialect/Shape/canonicalize.mlir b/mlir/test/Dialect/Shape/canonicalize.mlir index 12031822..aec5f32 100644 --- a/mlir/test/Dialect/Shape/canonicalize.mlir +++ b/mlir/test/Dialect/Shape/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt -split-input-file -allow-unregistered-dialect -canonicalize %s | FileCheck %s +// RUN: mlir-opt -split-input-file -allow-unregistered-dialect -canonicalize="test-convergence" %s | FileCheck %s // CHECK-LABEL: func @f func.func @f(%arg0: tensor<2x3x4xf32>) -> tensor<3xindex> { diff --git a/mlir/test/Dialect/Tensor/canonicalize.mlir b/mlir/test/Dialect/Tensor/canonicalize.mlir index fed2ca7..2b11a33 100644 --- a/mlir/test/Dialect/Tensor/canonicalize.mlir +++ b/mlir/test/Dialect/Tensor/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt %s -split-input-file -canonicalize | FileCheck %s +// RUN: mlir-opt %s -split-input-file -canonicalize="test-convergence" | FileCheck %s // Checks that NOP casts are removed. // CHECK-LABEL: cast_values diff --git a/mlir/test/Dialect/Tosa/canonicalize.mlir b/mlir/test/Dialect/Tosa/canonicalize.mlir index 7eea232..7464334 100644 --- a/mlir/test/Dialect/Tosa/canonicalize.mlir +++ b/mlir/test/Dialect/Tosa/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt --canonicalize %s | FileCheck %s +// RUN: mlir-opt -canonicalize="test-convergence" %s | FileCheck %s // CHECK-LABEL: @argmax_nofold func.func @argmax_nofold(%arg0: tensor) -> tensor { diff --git a/mlir/test/Dialect/Vector/canonicalize.mlir b/mlir/test/Dialect/Vector/canonicalize.mlir index 2ebe2d7..1990b89 100644 --- a/mlir/test/Dialect/Vector/canonicalize.mlir +++ b/mlir/test/Dialect/Vector/canonicalize.mlir @@ -1,6 +1,4 @@ -// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize))' -split-input-file -allow-unregistered-dialect | FileCheck %s - -// ----- +// RUN: mlir-opt %s -canonicalize="test-convergence" -split-input-file -allow-unregistered-dialect | FileCheck %s // CHECK-LABEL: create_vector_mask_to_constant_mask func.func @create_vector_mask_to_constant_mask() -> (vector<4x3xi1>) { diff --git a/mlir/test/Pass/run-reproducer.mlir b/mlir/test/Pass/run-reproducer.mlir index 3a958f8..903fd69 100644 --- a/mlir/test/Pass/run-reproducer.mlir +++ b/mlir/test/Pass/run-reproducer.mlir @@ -14,7 +14,7 @@ func.func @bar() { external_resources: { mlir_reproducer: { verify_each: true, - // CHECK: builtin.module(func.func(cse,canonicalize{ max-iterations=1 max-num-rewrites=-1 region-simplify=false top-down=false})) + // CHECK: builtin.module(func.func(cse,canonicalize{ max-iterations=1 max-num-rewrites=-1 region-simplify=false test-convergence=false top-down=false})) pipeline: "builtin.module(func.func(cse,canonicalize{max-iterations=1 max-num-rewrites=-1 region-simplify=false top-down=false}))", disable_threading: true } diff --git a/mlir/test/Transforms/canonicalize.mlir b/mlir/test/Transforms/canonicalize.mlir index df1555d..5cc0eb5 100644 --- a/mlir/test/Transforms/canonicalize.mlir +++ b/mlir/test/Transforms/canonicalize.mlir @@ -1,4 +1,4 @@ -// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline='builtin.module(func.func(canonicalize))' -split-input-file | FileCheck %s +// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' -split-input-file | FileCheck %s // CHECK-LABEL: func @test_subi_zero func.func @test_subi_zero(%arg0: i32) -> i32 { -- 2.7.4