Extend `getBackwardSlice` to track values captured from above #113478

IanWood1 · 2024-10-23T17:35:28Z

This change modifies getBackwardSlice to track values captures by the regions of each operation that it traverses. Ignoring values captured from a parent region may lead to an incomplete program slice. However, there seems to be logic that depends on not traversing captured values, so this change preserves the default behavior by hiding this logic behind the omitUsesFromAbove flag.

I'm looking for feedback on how to add this functionality (which I think should be default) without breaking users of getBackwardSlice

`getBackwardsSlice` should track values captured by each op's region that it traverses, and follow those defs. However, there might be logic that depends on not traversing captured values so this change preserves the default behavior by hiding this logic behind the `omitUsesFromAbove` flag.

llvmbot · 2024-10-23T17:46:59Z

@llvm/pr-subscribers-mlir

Author: Ian Wood (IanWood1)

Changes

This change modifies getBackwardSlice to track values captures by the regions of each operation that it traverses. Ignoring values captured from a parent region may lead to an incomplete program slice. However, there seems to be logic that depends on not traversing captured values, so this change preserves the default behavior by hiding this logic behind the omitUsesFromAbove flag.

I'm looking for feedback on how to add this functionality (which I think should be default) without breaking users of getBackwardSlice

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

2 Files Affected:

(modified) mlir/include/mlir/Analysis/SliceAnalysis.h (+5)
(modified) mlir/lib/Analysis/SliceAnalysis.cpp (+14)

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 99279fdfe427c8..a4f5d937cd51da 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -47,6 +47,11 @@ 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 2b1cf411ceeeeb..d07ae7b3ffa2c7 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -16,6 +16,7 @@
 #include "mlir/IR/Operation.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Transforms/RegionUtils.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
@@ -115,6 +116,19 @@ static void getBackwardSliceImpl(Operation *op,
     }
   }
 
+  // Visit values that are defined above.
+  if (!options.omitUsesFromAbove) {
+    visitUsedValuesDefinedAbove(op->getRegions(), [&](OpOperand *operand) {
+      if (Operation *definingOp = operand->get().getDefiningOp()) {
+        getBackwardSliceImpl(definingOp, backwardSlice, options);
+        return;
+      }
+      Operation *bbAargOwner =
+          cast<BlockArgument>(operand->get()).getOwner()->getParentOp();
+      getBackwardSliceImpl(bbAargOwner, backwardSlice, options);
+    });
+  }
+
   backwardSlice->insert(op);
 }

The reverted commit does not handle when the "consumer" uses a value defined above. See #18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (#18855) to fix this by including values used in an ops region in the backwards slice, but It is waiting on upstream changes to `getBackwardSlice`. Currently, the PR is using a wrapper around `getBackwardSlice` to acheive the same effect, but this will be updated once the upstream change lands (llvm/llvm-project#113478) Reverts #18551 --------- Signed-off-by: Ian Wood <[email protected]>

MaheshRavishankar

This looks good to me. You should add a test though. There are examples of tests for getBackwardSlice in mlir/test folder.

mlir/lib/Analysis/SliceAnalysis.cpp

joker-eph · 2024-10-29T12:47:13Z

mlir/lib/Analysis/SliceAnalysis.cpp

+        getBackwardSliceImpl(definingOp, backwardSlice, options);
+        return;
+      }
+      Operation *bbAargOwner =


Suggested change

Operation *bbAargOwner =

if (options.omitBlockArguments)

return;

Operation *bbAargOwner =

I realized that I was duplicating all the logic within the for loop going over the operands. Instead, I changed it to iterate over a SetVector of the operands + used values defined above, which decreases logic duplication. However, this does mean we have to allocate space to store the operands in the SetVector. But maybe this is fine?

joker-eph · 2024-10-30T20:07:43Z

mlir/lib/Analysis/SliceAnalysis.cpp

+    getUsedValuesDefinedAbove(op->getRegions(), valuesToFollow);
+  }
+
+  for (const auto &en : llvm::enumerate(valuesToFollow)) {


The way you can make it "free" when there are no regions is to define this code as a lambda that operates on an ValueRange.

auto processValues = [&] (ValueRange valuesToFollow) { ... };

And then dispatch like this:

processValues(op->getOperands()); if (!options.omitUsesFromAbove) { SetVector<Value> valuesToFollow; getUsedValuesDefinedAbove(op->getRegions(), valuesToFollow); processValues(valuesToFollow); }

Thank you, that's a good solution.

I tweaked it a bit to take a Value so it can get passed to visitValuesDefinedAbove which takes a lambda with an OpOperand* arg. This prevents storing the values in a set (backwardSlice->count ensures they are still only visited once)

The reverted commit does not handle when the "consumer" uses a value defined above. See #18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (#18855) to fix this by including values used in an ops region in the backwards slice, but It is waiting on upstream changes to `getBackwardSlice`. Currently, the PR is using a wrapper around `getBackwardSlice` to acheive the same effect, but this will be updated once the upstream change lands (llvm/llvm-project#113478) Reverts #18551 --------- Signed-off-by: Ian Wood <[email protected]> Signed-off-by: Elias Joseph <[email protected]>

…#113478)" This reverts commit 1bc58a2.

#114432) Reverts #113478 Bot is broken when building with shared libs.

joker-eph · 2024-10-31T17:31:31Z

I reverted because the mlir-nvidia bot was broken:

FAILED: lib/libMLIRAnalysis.so.20.0git 
: && /usr/bin/clang++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wundef -Werror=mismatched-tags -Werror=global-constructors -O3 -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete -fuse-ld=lld -Wl,--color-diagnostics   -Wl,--gc-sections -shared -Wl,-soname,libMLIRAnalysis.so.20.0git -o lib/libMLIRAnalysis.so.20.0git tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/AliasAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/CallGraph.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlowFramework.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataLayoutAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/FlatLinearValueConstraints.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/Liveness.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/CFGLoopInfo.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/SliceAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/SliceWalk.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/TopologicalSortUtils.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/AliasAnalysis/LocalAliasAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/ConstantPropagationAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/DeadCodeAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/DenseAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/IntegerRangeAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/LivenessAnalysis.cpp.o tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/DataFlow/SparseAnalysis.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib:/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib:"  lib/libMLIRCallInterfaces.so.20.0git  lib/libMLIRControlFlowInterfaces.so.20.0git  lib/libMLIRDataLayoutInterfaces.so.20.0git  lib/libMLIRInferIntRangeInterface.so.20.0git  lib/libMLIRInferTypeOpInterface.so.20.0git  lib/libMLIRLoopLikeInterface.so.20.0git  lib/libMLIRPresburger.so.20.0git  lib/libMLIRSideEffectInterfaces.so.20.0git  lib/libMLIRViewLikeInterface.so.20.0git  lib/libMLIRFunctionInterfaces.so.20.0git  lib/libMLIRIR.so.20.0git  lib/libMLIRSupport.so.20.0git  lib/libLLVMSupport.so.20.0git  -Wl,-rpath-link,/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib && :
ld.lld: error: undefined symbol: mlir::visitUsedValuesDefinedAbove(llvm::MutableArrayRef<mlir::Region>, llvm::function_ref<void (mlir::OpOperand*)>)
>>> referenced by SliceAnalysis.cpp
>>>               tools/mlir/lib/Analysis/CMakeFiles/obj.MLIRAnalysis.dir/SliceAnalysis.cpp.o:(getBackwardSliceImpl(mlir::Operation*, llvm::SetVector<mlir::Operation*, llvm::SmallVector<mlir::Operation*, 0u>, llvm::DenseSet<mlir::Operation*, llvm::DenseMapInfo<mlir::Operation*, void> >, 0u>*, mlir::BackwardSliceOptions const&))

You should be able to reproduce with -DBUILD_SHARED_LIBS=ON

bartchr808 · 2024-10-31T18:07:12Z

Also FYI this has caused a different failure with Bazel

https://buildkite.com/llvm-project/upstream-bazel/builds/115143#0192e378-3caf-494c-8647-7f50d4a17f77

The fix is not trivial due to a dependency cycle. the Analysis target should depend on TransformUtils, however, TransformUtils already depends on Analysis (see BUILD.bazel).

IanWood1 · 2024-10-31T19:07:27Z

Also FYI this has caused a different failure with Bazel

buildkite.com/llvm-project/upstream-bazel/builds/115143#0192e378-3caf-494c-8647-7f50d4a17f77

The fix is not trivial due to a dependency cycle. the Analysis target should depend on TransformUtils, however, TransformUtils already depends on Analysis (see BUILD.bazel).

Yeah, looks like I forgot to update the cmake deps and didn't catch cyclic dependency. Also, new PR here #114452

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.

…13478) This change modifies `getBackwardSlice` to track values captures by the regions of each operation that it traverses. Ignoring values captured from a parent region may lead to an incomplete program slice. However, there seems to be logic that depends on not traversing captured values, so this change preserves the default behavior by hiding this logic behind the `omitUsesFromAbove` flag.

llvm#114432) Reverts llvm#113478 Bot is broken when building with shared libs.

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.

…13478) This change modifies `getBackwardSlice` to track values captures by the regions of each operation that it traverses. Ignoring values captured from a parent region may lead to an incomplete program slice. However, there seems to be logic that depends on not traversing captured values, so this change preserves the default behavior by hiding this logic behind the `omitUsesFromAbove` flag.

llvm#114432) Reverts llvm#113478 Bot is broken when building with shared libs.

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.

…g#18917) The reverted commit does not handle when the "consumer" uses a value defined above. See iree-org#18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (iree-org#18855) to fix this by including values used in an ops region in the backwards slice, but It is waiting on upstream changes to `getBackwardSlice`. Currently, the PR is using a wrapper around `getBackwardSlice` to acheive the same effect, but this will be updated once the upstream change lands (llvm/llvm-project#113478) Reverts iree-org#18551 --------- Signed-off-by: Ian Wood <[email protected]> Signed-off-by: Giacomo Serafini <[email protected]>

This PR reverts the changes in #5203 that prevents reordering across scf ops Passes `omitUsesFromAbove=false` introduced in llvm/llvm-project#113478 `mlir::getBackwardSlice()`. This ensures that the backward slice captures the values used from above, in ops with nested regions, like scf ops.

IanWood1 marked this pull request as ready for review October 23, 2024 17:46

llvmbot added the mlir label Oct 23, 2024

IanWood1 mentioned this pull request Oct 24, 2024

Improve FuseMultiUseElementwiseProducersPass iree-org/iree#18856

Closed

IanWood1 self-assigned this Oct 28, 2024

IanWood1 requested review from MaheshRavishankar and hanhanW October 28, 2024 16:27

IanWood1 mentioned this pull request Oct 28, 2024

Revert "[DispatchCreation] Extend multi-use producer fusion" iree-org/iree#18917

Merged

MaheshRavishankar reviewed Oct 29, 2024

View reviewed changes

MaheshRavishankar requested review from River707 and joker-eph October 29, 2024 04:20

joker-eph reviewed Oct 29, 2024

View reviewed changes

mlir/lib/Analysis/SliceAnalysis.cpp Outdated Show resolved Hide resolved

joker-eph reviewed Oct 29, 2024

View reviewed changes

Cleanup impl & add test

3e8b47a

joker-eph reviewed Oct 30, 2024

View reviewed changes

Use lambda to reuse logic

9f7ea3c

IanWood1 requested review from MaheshRavishankar and joker-eph October 30, 2024 20:43

joker-eph approved these changes Oct 30, 2024

View reviewed changes

IanWood1 merged commit 1bc58a2 into llvm:main Oct 31, 2024
8 checks passed

IanWood1 deleted the fix_getbackwardsslice branch October 31, 2024 14:47

joker-eph added a commit that referenced this pull request Oct 31, 2024

Revert "Extend getBackwardSlice to track values captured from above (…

d0422f6

…#113478)" This reverts commit 1bc58a2.

joker-eph mentioned this pull request Oct 31, 2024

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

Merged

joker-eph added a commit that referenced this pull request Oct 31, 2024

Revert "Extend getBackwardSlice to track values captured from above" (

a9a8351

#114432) Reverts #113478 Bot is broken when building with shared libs.

IanWood1 restored the fix_getbackwardsslice branch October 31, 2024 17:42

IanWood1 mentioned this pull request Oct 31, 2024

Reapply "Extend getBackwardSlice to track values captured… #114452

Merged

smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024

Revert "Extend getBackwardSlice to track values captured from above" (

386390d

llvm#114432) Reverts llvm#113478 Bot is broken when building with shared libs.

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

Revert "Extend getBackwardSlice to track values captured from above" (

dcdaf62

llvm#114432) Reverts llvm#113478 Bot is broken when building with shared libs.

karthik-man mentioned this pull request Dec 7, 2024

[AMD] Enable reordering across ops with nested regions triton-lang/triton#5363

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extend `getBackwardSlice` to track values captured from above #113478

Extend `getBackwardSlice` to track values captured from above #113478

Uh oh!

IanWood1 commented Oct 23, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 23, 2024

Uh oh!

MaheshRavishankar left a comment

Uh oh!

Uh oh!

joker-eph Oct 29, 2024

Uh oh!

IanWood1 Oct 30, 2024

Uh oh!

joker-eph Oct 30, 2024 •

edited

Loading

Uh oh!

IanWood1 Oct 30, 2024

Uh oh!

Uh oh!

joker-eph commented Oct 31, 2024

Uh oh!

bartchr808 commented Oct 31, 2024

Uh oh!

IanWood1 commented Oct 31, 2024

Uh oh!

Uh oh!

Extend getBackwardSlice to track values captured from above #113478

Extend getBackwardSlice to track values captured from above #113478

Uh oh!

Conversation

IanWood1 commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 23, 2024

Uh oh!

MaheshRavishankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joker-eph Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

IanWood1 Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

joker-eph Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanWood1 Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joker-eph commented Oct 31, 2024

Uh oh!

bartchr808 commented Oct 31, 2024

Uh oh!

IanWood1 commented Oct 31, 2024

Uh oh!

Uh oh!

Extend `getBackwardSlice` to track values captured from above #113478

Extend `getBackwardSlice` to track values captured from above #113478

IanWood1 commented Oct 23, 2024 •

edited

Loading

joker-eph Oct 30, 2024 •

edited

Loading