Revert "[MLIR] getBackwardSlice: don't bail on ops that are IsolatedFromAbove"

[IanWood1]

Reverts #158135

Breaks MLIR Nvidia build: lab.llvm.org/buildbot#/builders/116/builds/18719

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Ian Wood (IanWood1)

Changes

Reverts llvm/llvm-project#158135

---

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

4 Files Affected:

• (modified) mlir/lib/Analysis/SliceAnalysis.cpp (+3-5)
• (modified) mlir/test/Transforms/move-operation-deps.mlir (-28)
• (modified) mlir/test/lib/Dialect/Test/TestOps.td (+1-2)
• (modified) mlir/test/lib/Transforms/TestMakeIsolatedFromAbove.cpp (+2-2)

diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 12dff19ed31d3..7037fa644c7be 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -109,7 +109,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
                                           DenseSet<Operation *> &visited,
                                           SetVector<Operation *> *backwardSlice,
                                           const BackwardSliceOptions &options) {
-  if (!op)
+  if (!op || op->hasTrait<OpTrait::IsIsolatedFromAbove>())
     return success();
 
   // Evaluate whether we should keep this def.
@@ -136,8 +136,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
       // blocks of parentOp, which are not technically backward unless they flow
       // into us. For now, just bail.
       if (parentOp && backwardSlice->count(parentOp) == 0) {
-        if (!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
-            parentOp->getNumRegions() == 1 &&
+        if (parentOp->getNumRegions() == 1 &&
             parentOp->getRegion(0).hasOneBlock()) {
           return getBackwardSliceImpl(parentOp, visited, backwardSlice,
                                       options);
@@ -151,8 +150,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
 
   bool succeeded = true;
 
-  if (!options.omitUsesFromAbove &&
-      !op->hasTrait<OpTrait::IsIsolatedFromAbove>()) {
+  if (!options.omitUsesFromAbove) {
     llvm::for_each(op->getRegions(), [&](Region &region) {
       // Walk this region recursively to collect the regions that descend from
       // this op's nested regions (inclusive).
diff --git a/mlir/test/Transforms/move-operation-deps.mlir b/mlir/test/Transforms/move-operation-deps.mlir
index 75d8386d520ee..aa7b5dc2a240a 100644
--- a/mlir/test/Transforms/move-operation-deps.mlir
+++ b/mlir/test/Transforms/move-operation-deps.mlir
@@ -460,31 +460,3 @@ module attributes {transform.with_named_sequence} {
     transform.yield
   }
 }
-
-// -----
-
-func.func @move_isolated_from_above() -> () {
-  %1 = "before"() : () -> (f32)
-  %2 = "moved0"() : () -> (f32)
-  %3 = test.isolated_one_region_op %2 {} : f32 -> f32
-  %4 = "moved1"(%3) : (f32) -> (f32)
-  return
-}
-// CHECK-LABEL: func @move_isolated_from_above()
-//       CHECK:   %[[MOVED0:.+]] = "moved0"
-//       CHECK:   %[[ISOLATED:.+]] = test.isolated_one_region_op %[[MOVED0]]
-//       CHECK:   %[[MOVED1:.+]] = "moved1"(%[[ISOLATED]])
-//       CHECK:   %[[BEFORE:.+]] = "before"
-
-module attributes {transform.with_named_sequence} {
-  transform.named_sequence @__transform_main(%arg0 : !transform.any_op {transform.readonly}) {
-    %op1 = transform.structured.match ops{["before"]} in %arg0
-        : (!transform.any_op) -> !transform.any_op
-    %op2 = transform.structured.match ops{["moved1"]} in %arg0
-        : (!transform.any_op) -> !transform.any_op
-    %v1 = transform.get_result %op2[0] : (!transform.any_op) -> !transform.any_value
-    transform.test.move_value_defns %v1 before %op1
-        : (!transform.any_value), !transform.any_op
-    transform.yield
-  }
-}
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index d9bbb3261febc..5564264ed8b0b 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -552,10 +552,9 @@ def OneRegionWithOperandsOp : TEST_Op<"one_region_with_operands_op", []> {
 
 def IsolatedOneRegionOp : TEST_Op<"isolated_one_region_op", [IsolatedFromAbove]> {
   let arguments = (ins Variadic<AnyType>:$operands);
-  let results = (outs Variadic<AnyType>:$results);
   let regions = (region AnyRegion:$my_region);
   let assemblyFormat = [{
-    attr-dict-with-keyword $operands $my_region `:` type($operands) `->` type($results)
+    attr-dict-with-keyword $operands $my_region `:` type($operands)
   }];
 }
 
diff --git a/mlir/test/lib/Transforms/TestMakeIsolatedFromAbove.cpp b/mlir/test/lib/Transforms/TestMakeIsolatedFromAbove.cpp
index f7bde79274e91..c1fb70605ab46 100644
--- a/mlir/test/lib/Transforms/TestMakeIsolatedFromAbove.cpp
+++ b/mlir/test/lib/Transforms/TestMakeIsolatedFromAbove.cpp
@@ -27,8 +27,8 @@ makeIsolatedFromAboveImpl(RewriterBase &rewriter,
       makeRegionIsolatedFromAbove(rewriter, region, callBack);
   SmallVector<Value> operands = regionOp.getOperands();
   operands.append(capturedValues);
-  auto isolatedRegionOp = test::IsolatedOneRegionOp::create(
-      rewriter, regionOp.getLoc(), TypeRange(), operands);
+  auto isolatedRegionOp =
+      test::IsolatedOneRegionOp::create(rewriter, regionOp.getLoc(), operands);
   rewriter.inlineRegionBefore(region, isolatedRegionOp.getRegion(),
                               isolatedRegionOp.getRegion().begin());
   rewriter.eraseOp(regionOp);

[joker-eph]

Please add the revert reason in the description

[IanWood1]

I think this is the same flaky test I've seen in the past. I reran the build and it passed: https://lab.llvm.org/buildbot/#/builders/116/builds/18721