Skip to content

Revert "Extend getBackwardSlice to track values captured from above" #114432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

joker-eph
Copy link
Collaborator

Reverts #113478

Bot is broken when building with shared libs.

@joker-eph joker-eph added the skip-precommit-approval PR for CI feedback, not intended for review label Oct 31, 2024
@joker-eph joker-eph requested a review from IanWood1 October 31, 2024 17:28
@joker-eph joker-eph merged commit a9a8351 into main Oct 31, 2024
5 of 7 checks passed
@joker-eph joker-eph deleted the revert-113478-fix_getbackwardsslice branch October 31, 2024 17:29
@llvmbot llvmbot added the mlir label Oct 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

Changes

Reverts llvm/llvm-project#113478

Bot is broken when building with shared libs.


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

4 Files Affected:

  • (modified) mlir/include/mlir/Analysis/SliceAnalysis.h (-5)
  • (modified) mlir/lib/Analysis/SliceAnalysis.cpp (+5-13)
  • (modified) mlir/test/IR/slice.mlir (+1-27)
  • (modified) mlir/test/lib/IR/TestSlicing.cpp (-2)
diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index a4f5d937cd51da..99279fdfe427c8 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -47,11 +47,6 @@ struct BackwardSliceOptions : public SliceOptions {
   /// backward slice computation traverses block arguments and asserts that the
   /// parent op has a single region with a single block.
   bool omitBlockArguments = false;
-
-  /// When omitUsesFromAbove is true, the backward slice computation omits
-  /// traversing values that are captured from above.
-  /// TODO: this should default to `false` after users have been updated.
-  bool omitUsesFromAbove = true;
 };
 
 using ForwardSliceOptions = SliceOptions;
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 7ec999fa0370f9..2b1cf411ceeeeb 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -16,8 +16,6 @@
 #include "mlir/IR/Operation.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "mlir/Support/LLVM.h"
-#include "mlir/Transforms/RegionUtils.h"
-#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
@@ -93,13 +91,14 @@ static void getBackwardSliceImpl(Operation *op,
   if (options.filter && !options.filter(op))
     return;
 
-  auto processValue = [&](Value value) {
-    if (auto *definingOp = value.getDefiningOp()) {
+  for (const auto &en : llvm::enumerate(op->getOperands())) {
+    auto operand = en.value();
+    if (auto *definingOp = operand.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0)
         getBackwardSliceImpl(definingOp, backwardSlice, options);
-    } else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
+    } else if (auto blockArg = dyn_cast<BlockArgument>(operand)) {
       if (options.omitBlockArguments)
-        return;
+        continue;
 
       Block *block = blockArg.getOwner();
       Operation *parentOp = block->getParentOp();
@@ -114,14 +113,7 @@ static void getBackwardSliceImpl(Operation *op,
     } else {
       llvm_unreachable("No definingOp and not a block argument.");
     }
-  };
-
-  if (!options.omitUsesFromAbove) {
-    visitUsedValuesDefinedAbove(op->getRegions(), [&](OpOperand *operand) {
-      processValue(operand->get());
-    });
   }
-  llvm::for_each(op->getOperands(), processValue);
 
   backwardSlice->insert(op);
 }
diff --git a/mlir/test/IR/slice.mlir b/mlir/test/IR/slice.mlir
index 87d446c8f415af..0a32a0f231baf2 100644
--- a/mlir/test/IR/slice.mlir
+++ b/mlir/test/IR/slice.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -slice-analysis-test -split-input-file %s | FileCheck %s
+// RUN: mlir-opt -slice-analysis-test %s | FileCheck %s
 
 func.func @slicing_linalg_op(%arg0 : index, %arg1 : index, %arg2 : index) {
   %a = memref.alloc(%arg0, %arg2) : memref<?x?xf32>
@@ -33,29 +33,3 @@ func.func @slicing_linalg_op(%arg0 : index, %arg1 : index, %arg2 : index) {
 //   CHECK-DAG:   %[[B:.+]] = memref.alloc(%[[ARG2]], %[[ARG1]]) : memref<?x?xf32>
 //   CHECK-DAG:   %[[C:.+]] = memref.alloc(%[[ARG0]], %[[ARG1]]) : memref<?x?xf32>
 //       CHECK:   return
-
-// -----
-
-#map = affine_map<(d0, d1) -> (d0, d1)>
-func.func @slice_use_from_above(%arg0: tensor<5x5xf32>, %arg1: tensor<5x5xf32>) {
-  %0 = linalg.generic {indexing_maps = [#map, #map], iterator_types = ["parallel", "parallel"]} ins(%arg0 : tensor<5x5xf32>) outs(%arg1 : tensor<5x5xf32>) {
-  ^bb0(%in: f32, %out: f32):
-    %2 = arith.addf %in, %in : f32
-    linalg.yield %2 : f32
-  } -> tensor<5x5xf32>
-  %collapsed = tensor.collapse_shape %0 [[0, 1]] : tensor<5x5xf32> into tensor<25xf32>
-  %1 = linalg.generic {indexing_maps = [#map, #map], iterator_types = ["parallel", "parallel"]} ins(%0 : tensor<5x5xf32>) outs(%arg1 : tensor<5x5xf32>) {
-  ^bb0(%in: f32, %out: f32):
-    %c2 = arith.constant 2 : index
-    %extracted = tensor.extract %collapsed[%c2] : tensor<25xf32>
-    %2 = arith.addf %extracted, %extracted : f32
-    linalg.yield %2 : f32
-  } -> tensor<5x5xf32>
-  return
-}
-
-// CHECK-LABEL: func @slice_use_from_above__backward_slice__0
-//  CHECK-SAME:   %[[ARG0:[a-zA-Z0-9_]+]]: tensor 
-//       CHECK:   %[[A:.+]] = linalg.generic {{.*}} ins(%[[ARG0]]
-//       CHECK:   %[[B:.+]] = tensor.collapse_shape %[[A]]
-//       CHECK:   return
diff --git a/mlir/test/lib/IR/TestSlicing.cpp b/mlir/test/lib/IR/TestSlicing.cpp
index e99d5976d6d9df..c3d0d151c6d24d 100644
--- a/mlir/test/lib/IR/TestSlicing.cpp
+++ b/mlir/test/lib/IR/TestSlicing.cpp
@@ -39,8 +39,6 @@ static LogicalResult createBackwardSliceFunction(Operation *op,
   SetVector<Operation *> slice;
   BackwardSliceOptions options;
   options.omitBlockArguments = omitBlockArguments;
-  // TODO: Make this default.
-  options.omitUsesFromAbove = false;
   getBackwardSlice(op, &slice, options);
   for (Operation *slicedOp : slice)
     builder.clone(*slicedOp, mapper);

IanWood1 added a commit to IanWood1/llvm-project that referenced this pull request Oct 31, 2024
IanWood1 added a commit that referenced this pull request Nov 1, 2024
This commit fixes the failure in the original PR when building with
shared libs. The problem is that `visitUsedValuesDefinedAbove` is
defined in `MLIRTransformUtils`, but that lib depends on this lib
(`MLIRAnalysis`). To fix, I dropped the use of
`visitUsedValuesDefinedAbove` and use `Region::walk` to traverse values
defined above.

Reapplies PR #113478
Reverts PR #114432

This reverts commit a9a8351.
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
This commit fixes the failure in the original PR when building with
shared libs. The problem is that `visitUsedValuesDefinedAbove` is
defined in `MLIRTransformUtils`, but that lib depends on this lib
(`MLIRAnalysis`). To fix, I dropped the use of
`visitUsedValuesDefinedAbove` and use `Region::walk` to traverse values
defined above.

Reapplies PR llvm#113478
Reverts PR llvm#114432

This reverts commit a9a8351.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
This commit fixes the failure in the original PR when building with
shared libs. The problem is that `visitUsedValuesDefinedAbove` is
defined in `MLIRTransformUtils`, but that lib depends on this lib
(`MLIRAnalysis`). To fix, I dropped the use of
`visitUsedValuesDefinedAbove` and use `Region::walk` to traverse values
defined above.

Reapplies PR llvm#113478
Reverts PR llvm#114432

This reverts commit a9a8351.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir skip-precommit-approval PR for CI feedback, not intended for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants