Skip to content

Conversation

matthias-springer
Copy link
Member

The dialect conversion-based bufferization passes have been migrated to One-Shot Bufferize about two years ago. To clean up the code base, this commit removes the scf-bufferize pass, one of the few remaining parts of the old infrastructure. Most bufferization passes have already been removed.

Note for LLVM integration: If you depend on this pass, migrate to One-Shot Bufferize or copy the pass to your codebase.

@llvmbot
Copy link
Member

llvmbot commented Oct 27, 2024

@llvm/pr-subscribers-mlir-scf

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

The dialect conversion-based bufferization passes have been migrated to One-Shot Bufferize about two years ago. To clean up the code base, this commit removes the scf-bufferize pass, one of the few remaining parts of the old infrastructure. Most bufferization passes have already been removed.

Note for LLVM integration: If you depend on this pass, migrate to One-Shot Bufferize or copy the pass to your codebase.


Full diff: https://github.com/llvm/llvm-project/pull/113840.diff

5 Files Affected:

  • (modified) mlir/docs/Bufferization.md (+1-16)
  • (modified) mlir/include/mlir/Dialect/SCF/Transforms/Passes.h (-3)
  • (modified) mlir/include/mlir/Dialect/SCF/Transforms/Passes.td (-7)
  • (removed) mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp (-47)
  • (removed) mlir/test/Dialect/SCF/bufferize.mlir (-101)
diff --git a/mlir/docs/Bufferization.md b/mlir/docs/Bufferization.md
index d5a426e09e7ceb..7d38ebb38535c7 100644
--- a/mlir/docs/Bufferization.md
+++ b/mlir/docs/Bufferization.md
@@ -579,7 +579,6 @@ The code, slightly simplified and annotated, is reproduced here:
   // Partial bufferization passes.
   pm.addPass(createTensorConstantBufferizePass());
   pm.addNestedPass<func::FuncOp>(createTCPBufferizePass()); // Bufferizes the downstream `tcp` dialect.
-  pm.addNestedPass<func::FuncOp>(createSCFBufferizePass());
   pm.addNestedPass<func::FuncOp>(createLinalgBufferizePass());
   pm.addNestedPass<func::FuncOp>(createTensorBufferizePass());
   pm.addPass(createFuncBufferizePass());
@@ -596,7 +595,7 @@ must be module passes because they make changes to the top-level module.
 
 The bulk of the bufferization work is done by the function passes. Most of these
 passes are provided as part of the upstream MLIR distribution and bufferize
-their respective dialects (e.g. `scf-bufferize` bufferizes the `scf` dialect).
+their respective dialects (e.g. `abc-bufferize` bufferizes the `abc` dialect).
 The `tcp-bufferize` pass is an exception -- it is a partial bufferization pass
 used to bufferize the downstream `tcp` dialect, and fits in perfectly with all
 the other passes provided upstream.
@@ -694,20 +693,6 @@ which helps with this in general.
 
 ### Other partial bufferization examples
 
--   `scf-bufferize`
-    ([code](https://github.com/llvm/llvm-project/blob/bc8acf2ce8ad6e8c9b1d97b2e02d3f4ad26e1d9d/mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp#L1),
-    [test](https://github.com/llvm/llvm-project/blob/bc8acf2ce8ad6e8c9b1d97b2e02d3f4ad26e1d9d/mlir/test/Dialect/SCF/bufferize.mlir#L1))
-
-    -   Bufferizes ops from the `scf` dialect.
-    -   This is an example of how to bufferize ops that implement
-        `RegionBranchOpInterface` (that is, they use regions to represent
-        control flow).
-    -   The bulk of the work is done by
-        `lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp`
-        ([code](https://github.com/llvm/llvm-project/blob/daaaed6bb89044ac58a23f1bb1ccdd12342a5a58/mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp#L1)),
-        which is well-commented and covers how to correctly convert ops that
-        contain regions.
-
 -   `func-bufferize`
     ([code](https://github.com/llvm/llvm-project/blob/2f5715dc78328215d51d5664c72c632a6dac1046/mlir/lib/Dialect/Func/Transforms/FuncBufferize.cpp#L1),
     [test](https://github.com/llvm/llvm-project/blob/2f5715dc78328215d51d5664c72c632a6dac1046/mlir/test/Dialect/Func/func-bufferize.mlir#L1))
diff --git a/mlir/include/mlir/Dialect/SCF/Transforms/Passes.h b/mlir/include/mlir/Dialect/SCF/Transforms/Passes.h
index fb8411418ff9a0..b70599df6f5033 100644
--- a/mlir/include/mlir/Dialect/SCF/Transforms/Passes.h
+++ b/mlir/include/mlir/Dialect/SCF/Transforms/Passes.h
@@ -20,9 +20,6 @@ namespace mlir {
 #define GEN_PASS_DECL
 #include "mlir/Dialect/SCF/Transforms/Passes.h.inc"
 
-/// Creates a pass that bufferizes the SCF dialect.
-std::unique_ptr<Pass> createSCFBufferizePass();
-
 /// Creates a pass that specializes for loop for unrolling and
 /// vectorization.
 std::unique_ptr<Pass> createForLoopSpecializationPass();
diff --git a/mlir/include/mlir/Dialect/SCF/Transforms/Passes.td b/mlir/include/mlir/Dialect/SCF/Transforms/Passes.td
index 53d1ae10dc87d8..6e5ef96c450aa4 100644
--- a/mlir/include/mlir/Dialect/SCF/Transforms/Passes.td
+++ b/mlir/include/mlir/Dialect/SCF/Transforms/Passes.td
@@ -11,13 +11,6 @@
 
 include "mlir/Pass/PassBase.td"
 
-def SCFBufferize : Pass<"scf-bufferize"> {
-  let summary = "Bufferize the scf dialect.";
-  let constructor = "mlir::createSCFBufferizePass()";
-  let dependentDialects = ["bufferization::BufferizationDialect",
-                           "memref::MemRefDialect"];
-}
-
 // Note: Making these canonicalization patterns would require a dependency
 // of the SCF dialect on the Affine/Tensor/MemRef dialects or vice versa.
 def SCFForLoopCanonicalization
diff --git a/mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp b/mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp
deleted file mode 100644
index 21c618ab633f60..00000000000000
--- a/mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp
+++ /dev/null
@@ -1,47 +0,0 @@
-//===- Bufferize.cpp - scf bufferize pass ---------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "mlir/Dialect/SCF/Transforms/Passes.h"
-
-#include "mlir/Dialect/Bufferization/IR/Bufferization.h"
-#include "mlir/Dialect/Bufferization/Transforms/Bufferize.h"
-#include "mlir/Dialect/MemRef/IR/MemRef.h"
-#include "mlir/Dialect/SCF/IR/SCF.h"
-#include "mlir/Dialect/SCF/Transforms/Patterns.h"
-#include "mlir/Transforms/DialectConversion.h"
-
-namespace mlir {
-#define GEN_PASS_DEF_SCFBUFFERIZE
-#include "mlir/Dialect/SCF/Transforms/Passes.h.inc"
-} // namespace mlir
-
-using namespace mlir;
-using namespace mlir::scf;
-
-namespace {
-struct SCFBufferizePass : public impl::SCFBufferizeBase<SCFBufferizePass> {
-  void runOnOperation() override {
-    auto *func = getOperation();
-    auto *context = &getContext();
-
-    bufferization::BufferizeTypeConverter typeConverter;
-    RewritePatternSet patterns(context);
-    ConversionTarget target(*context);
-
-    bufferization::populateBufferizeMaterializationLegality(target);
-    populateSCFStructuralTypeConversionsAndLegality(typeConverter, patterns,
-                                                    target);
-    if (failed(applyPartialConversion(func, target, std::move(patterns))))
-      return signalPassFailure();
-  };
-};
-} // namespace
-
-std::unique_ptr<Pass> mlir::createSCFBufferizePass() {
-  return std::make_unique<SCFBufferizePass>();
-}
diff --git a/mlir/test/Dialect/SCF/bufferize.mlir b/mlir/test/Dialect/SCF/bufferize.mlir
deleted file mode 100644
index ff1612310255a0..00000000000000
--- a/mlir/test/Dialect/SCF/bufferize.mlir
+++ /dev/null
@@ -1,101 +0,0 @@
-// RUN: mlir-opt %s -scf-bufferize | FileCheck %s
-
-// CHECK-LABEL:   func @if(
-// CHECK-SAME:             %[[PRED:.*]]: i1,
-// CHECK-SAME:             %[[TRUE_TENSOR:.*]]: tensor<?xf32>,
-// CHECK-SAME:             %[[FALSE_TENSOR:.*]]: tensor<?xf32>) -> tensor<?xf32> {
-// CHECK-DAG:       %[[TRUE_MEMREF:.*]] = bufferization.to_memref %[[TRUE_TENSOR]] : memref<?xf32>
-// CHECK-DAG:       %[[FALSE_MEMREF:.*]] = bufferization.to_memref %[[FALSE_TENSOR]] : memref<?xf32>
-// CHECK:           %[[RESULT_MEMREF:.*]] = scf.if %[[PRED]] -> (memref<?xf32>) {
-// CHECK:             scf.yield %[[TRUE_MEMREF]] : memref<?xf32>
-// CHECK:           } else {
-// CHECK:             scf.yield %[[FALSE_MEMREF]] : memref<?xf32>
-// CHECK:           }
-// CHECK:           %[[RESULT_TENSOR:.*]] = bufferization.to_tensor %[[RESULT_MEMREF:.*]] : memref<?xf32>
-// CHECK:           return %[[RESULT_TENSOR]] : tensor<?xf32>
-// CHECK:         }
-func.func @if(%pred: i1, %true_val: tensor<?xf32>, %false_val: tensor<?xf32>) -> tensor<?xf32> {
-  %0 = scf.if %pred -> (tensor<?xf32>) {
-    scf.yield %true_val : tensor<?xf32>
-  } else {
-    scf.yield %false_val : tensor<?xf32>
-  }
-  return %0 : tensor<?xf32>
-}
-
-// CHECK-LABEL:   func @for(
-// CHECK-SAME:              %[[TENSOR:.*]]: tensor<f32>,
-// CHECK-SAME:              %[[LB:.*]]: index, %[[UB:.*]]: index,
-// CHECK-SAME:              %[[STEP:.*]]: index) -> tensor<f32> {
-// CHECK:           %[[MEMREF:.*]] = bufferization.to_memref %[[TENSOR]] : memref<f32>
-// CHECK:           %[[RESULT_MEMREF:.*]] = scf.for %[[VAL_6:.*]] = %[[LB]] to %[[UB]] step %[[STEP]] iter_args(%[[ITER:.*]] = %[[MEMREF]]) -> (memref<f32>) {
-// CHECK:             scf.yield %[[ITER]] : memref<f32>
-// CHECK:           } {some_attr}
-// CHECK:           %[[VAL_8:.*]] = bufferization.to_tensor %[[VAL_9:.*]] : memref<f32>
-// CHECK:           return %[[VAL_8]] : tensor<f32>
-// CHECK:         }
-func.func @for(%arg0: tensor<f32>, %lb: index, %ub: index, %step: index) -> tensor<f32> {
-  %ret = scf.for %iv = %lb to %ub step %step iter_args(%iter = %arg0) -> tensor<f32> {
-    scf.yield %iter : tensor<f32>
-  } {some_attr}
-  return %ret : tensor<f32>
-}
-
-// Check whether this converts at all.
-//
-// It would previously fail altogether.
-// CHECK-LABEL:   func @if_correct_recursive_legalization_behavior
-// CHECK: "test.munge_tensor"
-func.func @if_correct_recursive_legalization_behavior(%pred: i1, %tensor: tensor<f32>) -> tensor<f32> {
-  %0 = scf.if %pred -> (tensor<f32>) {
-    %1 = "test.munge_tensor"(%tensor) : (tensor<f32>) -> (tensor<f32>)
-    scf.yield %1: tensor<f32>
-  } else {
-    %1 = "test.munge_tensor"(%tensor) : (tensor<f32>) -> (tensor<f32>)
-    scf.yield %1 : tensor<f32>
-  }
-  return %0 : tensor<f32>
-}
-
-// CHECK-LABEL:   func @for_correct_recursive_legalization_behavior(
-// CHECK-SAME:                                                      %[[TENSOR:.*]]: tensor<f32>,
-// CHECK-SAME:                                                      %[[INDEX:.*]]: index) -> tensor<f32> {
-// CHECK:           %[[MEMREF:.*]] = bufferization.to_memref %[[TENSOR]] : memref<f32>
-// CHECK:           %[[RESULT:.*]] = scf.for %[[IV:.*]] = %[[INDEX]] to %[[INDEX]] step %[[INDEX]] iter_args(%[[MEMREF_ITER:.*]] = %[[MEMREF]]) -> (memref<f32>) {
-// CHECK:             %[[TENSOR_ITER:.*]] = bufferization.to_tensor %[[MEMREF_ITER]] : memref<f32>
-// CHECK:             %[[TENSOR_MUNGED:.*]] = "test.munge_tensor"(%[[TENSOR_ITER]]) : (tensor<f32>) -> tensor<f32>
-// CHECK:             %[[MEMREF_MUNGED:.*]] = bufferization.to_memref %[[TENSOR_MUNGED]] : memref<f32>
-// CHECK:             scf.yield %[[MEMREF_MUNGED]] : memref<f32>
-// CHECK:           }
-// CHECK:           %[[TENSOR:.*]] = bufferization.to_tensor %[[RESULT:.*]] : memref<f32>
-// CHECK:           return %[[TENSOR]] : tensor<f32>
-// CHECK:         }
-func.func @for_correct_recursive_legalization_behavior(%arg0: tensor<f32>, %index: index) -> tensor<f32> {
-  %ret = scf.for %iv = %index to %index step %index iter_args(%iter = %arg0) -> tensor<f32> {
-    %0 = "test.munge_tensor"(%iter) : (tensor<f32>) -> (tensor<f32>)
-    scf.yield %0 : tensor<f32>
-  }
-  return %ret : tensor<f32>
-}
-
-// CHECK-LABEL:   func @bufferize_while(
-// CHECK-SAME: %[[ARG0:.*]]: i64, %[[ARG1:.*]]: i64, %[[ARG2:.*]]: tensor<f32>
-// CHECK: %[[M:.*]] = bufferization.to_memref %[[ARG2]] : memref<f32>
-// CHECK: %[[RES1:.*]]:3 = scf.while (%{{.*}} = %[[ARG0]], %{{.*}} = %[[M]]) : (i64, memref<f32>) -> (i64, i64, memref<f32>)
-// CHECK: scf.condition(%{{.*}}) %{{.*}}, %{{.*}}, %{{.*}} : i64, i64, memref<f32>
-// CHECK: ^bb0(%{{.*}}: i64, %{{.*}}: i64, %{{.*}}: memref<f32>):
-// CHECK: scf.yield %{{.*}}, %{{.*}} : i64, memref<f32>
-// CHECK:  %[[RES2:.*]] = bufferization.to_tensor %[[RES1]]#2 : memref<f32>
-// CHECK:  return %[[RES1]]#1, %[[RES2]] : i64, tensor<f32>
-func.func @bufferize_while(%arg0: i64, %arg1: i64, %arg2: tensor<f32>) -> (i64, tensor<f32>) {
-  %c2_i64 = arith.constant 2 : i64
-  %0:3 = scf.while (%arg3 = %arg0, %arg4 = %arg2) : (i64, tensor<f32>) -> (i64, i64, tensor<f32>) {
-    %1 = arith.cmpi slt, %arg3, %arg1 : i64
-    scf.condition(%1) %arg3, %arg3, %arg4 : i64, i64, tensor<f32>
-  } do {
-  ^bb0(%arg5: i64, %arg6: i64, %arg7: tensor<f32>):
-    %1 = arith.muli %arg6, %c2_i64 : i64
-    scf.yield %1, %arg7 : i64, tensor<f32>
-  }
-  return %0#1, %0#2 : i64, tensor<f32>
-}

@matthias-springer matthias-springer force-pushed the users/matthias-springer/remove_scf_bufferize branch from 834b9a0 to f54d9eb Compare October 27, 2024 22:24
Copy link
Contributor

@javedabsar1 javedabsar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since one-shot replaces scf-bufferize would it be better instead of deleting test/Dialect/SCF/bufferize.mlir to change RUN to --one-shot-bufferize. That way, a test which might catch errors doesnt get deleted.
Having said that, I actually tried it but got this error. Not sure why - but 100% sure you will know why:

../build/bin/mlir-opt  --one-shot-bufferize test/Dialect/SCF/bufferize.mlir
test/Dialect/SCF/bufferize.mlir:76:5: error: Yield operand #0 is not equivalent to the corresponding iter bbArg
    scf.yield %0 : tensor<f32>

@matthias-springer matthias-springer force-pushed the users/matthias-springer/remove_scf_bufferize branch from f54d9eb to d253d61 Compare October 28, 2024 00:16
@matthias-springer matthias-springer force-pushed the users/matthias-springer/remove_scf_bufferize branch from d253d61 to df9727c Compare October 28, 2024 00:18
@matthias-springer
Copy link
Member Author

Good point. We have separate test cases that run One-Shot Bufferize. One per dialect. They are called one-shot-bufferize.mlir etc. The bufferize.mlir test cases also run One-Shot Bufferize but without an analysis. Basically just the bufferize() interface method. I updated the SCF bufferize.mlir test file accordingly. There was also a minor bug in the SCF interface implementation (upcasting an AnalysisState to OneShotAnalysisState when the object was not a OneShotAnalysisState) that I fixed.

Copy link
Contributor

@javedabsar1 javedabsar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for fixing the scf interface bug. If you reckon that the test/Dialect/SCF/bufferize.mlir test is covered already under the other tests, please delete instead of duplication. I just wanted to draw your attention to the fail.

@matthias-springer
Copy link
Member Author

LGTM. Thanks for fixing the scf interface bug. If you reckon that the test/Dialect/SCF/bufferize.mlir test is covered already under the other tests, please delete instead of duplication. I just wanted to draw your attention to the fail.

Thx, it's good to have a test that does not run the analysis, so let's keep it.

@joker-eph
Copy link
Collaborator

LG, thanks for the cleanup!
Did you check that no documentation leftover references it?

@matthias-springer
Copy link
Member Author

Yes, I grepped for all occurrences of the pass name. There is still func-bufferize and finalizing-bufferize, which have to go (in separate PRs), then the remaining documentation of the dialect conversion-based bufferization infrastructure can be deleted.

@matthias-springer matthias-springer merged commit 1549a0c into main Oct 29, 2024
8 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/remove_scf_bufferize branch October 29, 2024 00:10
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
The dialect conversion-based bufferization passes have been migrated to
One-Shot Bufferize about two years ago. To clean up the code base, this
commit removes the `scf-bufferize` pass, one of the few remaining parts
of the old infrastructure. Most bufferization passes have already been
removed.

Note for LLVM integration: If you depend on this pass, migrate to
One-Shot Bufferize or copy the pass to your codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants