[mlir][IR] Add `InsertPoint::after(ValueRange)` #114940

matthias-springer · 2024-11-05T06:09:21Z

This commit adds a helper function InsertPoint::after. The function computes an insertion point that post-dominates the definition of all given values.

This functionality will be used in the merged 1:1 / 1:N dialect conversion to determine where to insert a builtin.unrealized_conversion_cast op. To ensure that the same cast can be "reused" for multiple consumer ops, the cast should be inserted as early as possible. (Alternatively, multiple cast ops could be inserted, right before each consumer op.)

TODO: Add test case.

Depends on #115587.

llvmbot · 2024-11-05T06:09:52Z

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

This commit adds a helper function to OpBuilder::InsertPoint to compute the earliest insertion point where a given list of SSA values is defined.

This functionality will be used in the merged 1:1 / 1:N dialect conversion to determine where to insert a builtin.unrealized_conversion_cast op. To ensure that the same cast can be "reused" for multiple consumer ops, the cast should be inserted as early as possible.

TODO: Add test case.

Depends on #114928.

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

2 Files Affected:

(modified) mlir/include/mlir/IR/Builders.h (+11)
(modified) mlir/lib/IR/Builders.cpp (+66)

diff --git a/mlir/include/mlir/IR/Builders.h b/mlir/include/mlir/IR/Builders.h
index 04a8bddc3cd59a..44079b4003de0c 100644
--- a/mlir/include/mlir/IR/Builders.h
+++ b/mlir/include/mlir/IR/Builders.h
@@ -334,6 +334,17 @@ class OpBuilder : public Builder {
   /// This class represents a saved insertion point.
   class InsertPoint {
   public:
+    /// Finds the closest insertion point where all given values are defined
+    /// and can be used without a dominance violation. Returns an "empty"
+    /// insertion point if there is no such insertion point.
+    static InsertPoint findClosest(ArrayRef<Value> values);
+
+    /// Creates a new insertion point right after the given value. If the given
+    /// value is a block argument, the insertion point points to the beginning
+    /// of the block. Otherwise, it points to the place right after the op that
+    /// defines the value.
+    static InsertPoint after(Value value);
+
     /// Creates a new insertion point which doesn't point to anything.
     InsertPoint() = default;
 
diff --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index 5397fbabc5c95e..e610b08df71cb1 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -641,3 +641,69 @@ void OpBuilder::cloneRegionBefore(Region &region, Region &parent,
 void OpBuilder::cloneRegionBefore(Region &region, Block *before) {
   cloneRegionBefore(region, *before->getParent(), before->getIterator());
 }
+
+//===----------------------------------------------------------------------===//
+// InsertPoint
+//===----------------------------------------------------------------------===//
+
+OpBuilder::InsertPoint OpBuilder::InsertPoint::after(Value value) {
+  if (auto blockArg = dyn_cast<BlockArgument>(value))
+    return InsertPoint(blockArg.getOwner(), blockArg.getOwner()->begin());
+  Operation *op = value.getDefiningOp();
+  return InsertPoint(op->getBlock(), ++op->getIterator());
+}
+
+/// Helper function that returns "true" if `a` is a proper ancestor of `b`.
+static bool isAncestor(Block *a, Block *b) {
+  return a->getParentOp()->isProperAncestor(b->getParentOp());
+}
+
+OpBuilder::InsertPoint OpBuilder::InsertPoint::findClosest(ArrayRef<Value> values) {
+  // Compute the insertion point after the first value.
+  assert(!values.empty() && "expected at least one value");
+  InsertPoint result = InsertPoint::after(values.front());
+
+  // Check all other values and update the insertion point as needed.
+  for (Value v : values.drop_front()) {
+    InsertPoint pt = InsertPoint::after(v);
+
+    if (pt.getBlock() == result.getBlock()) {
+      // Both values belong to the same block. Modify the iterator (but keep
+      // the block) if needed: take the later one of the two insertion points.
+      Block *block = pt.getBlock();
+      if (pt.getPoint() == block->end()) {
+        // `pt` points to the end of the block: take `pt`.
+        result = pt;
+        continue;
+      } else if (result.getPoint() == block->end()) {
+        // `result` points to the end of the block: nothing to do.
+        continue;
+      }
+      // Neither `pt` nor `result` point to the end of the block, so both
+      // iterators point to an operation. Set `result` to the later one of the
+      // two insertion point.
+      if (result.getPoint()->isBeforeInBlock(&*pt.getPoint()))
+        result = pt;
+      continue;
+    }
+
+    if (isAncestor(result.getBlock(), pt.getBlock())) {
+      // `result` is an ancestor of `pt`. Therefore, `pt` is a valid insertion
+      // point for `v` and all previous values.
+      result = pt;
+      continue;
+    }
+
+    if (isAncestor(pt.getBlock(), result.getBlock())) {
+      // `pt` is an ancestor of `result`. Therefore, `result` is a valid
+      // insertion point for `v` and all previous values.
+      continue;
+    }
+
+    // `pt` and `result` are in different subtrees: neither is an ancestor of
+    // the other. In that case, there is no suitable insertion point.
+    return InsertPoint();
+  }
+
+  return result;
+}

llvmbot · 2024-11-05T06:09:53Z

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

This commit adds a helper function to OpBuilder::InsertPoint to compute the earliest insertion point where a given list of SSA values is defined.

This functionality will be used in the merged 1:1 / 1:N dialect conversion to determine where to insert a builtin.unrealized_conversion_cast op. To ensure that the same cast can be "reused" for multiple consumer ops, the cast should be inserted as early as possible.

TODO: Add test case.

Depends on #114928.

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

2 Files Affected:

(modified) mlir/include/mlir/IR/Builders.h (+11)
(modified) mlir/lib/IR/Builders.cpp (+66)

diff --git a/mlir/include/mlir/IR/Builders.h b/mlir/include/mlir/IR/Builders.h
index 04a8bddc3cd59a..44079b4003de0c 100644
--- a/mlir/include/mlir/IR/Builders.h
+++ b/mlir/include/mlir/IR/Builders.h
@@ -334,6 +334,17 @@ class OpBuilder : public Builder {
   /// This class represents a saved insertion point.
   class InsertPoint {
   public:
+    /// Finds the closest insertion point where all given values are defined
+    /// and can be used without a dominance violation. Returns an "empty"
+    /// insertion point if there is no such insertion point.
+    static InsertPoint findClosest(ArrayRef<Value> values);
+
+    /// Creates a new insertion point right after the given value. If the given
+    /// value is a block argument, the insertion point points to the beginning
+    /// of the block. Otherwise, it points to the place right after the op that
+    /// defines the value.
+    static InsertPoint after(Value value);
+
     /// Creates a new insertion point which doesn't point to anything.
     InsertPoint() = default;
 
diff --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index 5397fbabc5c95e..e610b08df71cb1 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -641,3 +641,69 @@ void OpBuilder::cloneRegionBefore(Region &region, Region &parent,
 void OpBuilder::cloneRegionBefore(Region &region, Block *before) {
   cloneRegionBefore(region, *before->getParent(), before->getIterator());
 }
+
+//===----------------------------------------------------------------------===//
+// InsertPoint
+//===----------------------------------------------------------------------===//
+
+OpBuilder::InsertPoint OpBuilder::InsertPoint::after(Value value) {
+  if (auto blockArg = dyn_cast<BlockArgument>(value))
+    return InsertPoint(blockArg.getOwner(), blockArg.getOwner()->begin());
+  Operation *op = value.getDefiningOp();
+  return InsertPoint(op->getBlock(), ++op->getIterator());
+}
+
+/// Helper function that returns "true" if `a` is a proper ancestor of `b`.
+static bool isAncestor(Block *a, Block *b) {
+  return a->getParentOp()->isProperAncestor(b->getParentOp());
+}
+
+OpBuilder::InsertPoint OpBuilder::InsertPoint::findClosest(ArrayRef<Value> values) {
+  // Compute the insertion point after the first value.
+  assert(!values.empty() && "expected at least one value");
+  InsertPoint result = InsertPoint::after(values.front());
+
+  // Check all other values and update the insertion point as needed.
+  for (Value v : values.drop_front()) {
+    InsertPoint pt = InsertPoint::after(v);
+
+    if (pt.getBlock() == result.getBlock()) {
+      // Both values belong to the same block. Modify the iterator (but keep
+      // the block) if needed: take the later one of the two insertion points.
+      Block *block = pt.getBlock();
+      if (pt.getPoint() == block->end()) {
+        // `pt` points to the end of the block: take `pt`.
+        result = pt;
+        continue;
+      } else if (result.getPoint() == block->end()) {
+        // `result` points to the end of the block: nothing to do.
+        continue;
+      }
+      // Neither `pt` nor `result` point to the end of the block, so both
+      // iterators point to an operation. Set `result` to the later one of the
+      // two insertion point.
+      if (result.getPoint()->isBeforeInBlock(&*pt.getPoint()))
+        result = pt;
+      continue;
+    }
+
+    if (isAncestor(result.getBlock(), pt.getBlock())) {
+      // `result` is an ancestor of `pt`. Therefore, `pt` is a valid insertion
+      // point for `v` and all previous values.
+      result = pt;
+      continue;
+    }
+
+    if (isAncestor(pt.getBlock(), result.getBlock())) {
+      // `pt` is an ancestor of `result`. Therefore, `result` is a valid
+      // insertion point for `v` and all previous values.
+      continue;
+    }
+
+    // `pt` and `result` are in different subtrees: neither is an ancestor of
+    // the other. In that case, there is no suitable insertion point.
+    return InsertPoint();
+  }
+
+  return result;
+}

matthias-springer · 2024-11-05T06:11:00Z

I could put this helper function into DialectConversion.cpp, but I thought it could be useful in general. (I'm also trying to make the 1:N dialect conversion PR smaller before sending it for review.) What's unclear is how to test this. Maybe a unit test? What do you think?

github-actions · 2024-11-05T06:12:31Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mlir/lib/IR/Builders.cpp

matthias-springer · 2024-11-08T07:41:27Z

I'm trying to implement this in a different way: based on dominance, as you suggested. By adding a new DominanceInfo::dominates(Block*, Block::iterator, Block*, Block::iterator) function. That may also clean up other things inside of DominanceInfo. Still experimenting with the design... Marking this PR as "draft" until then.

And this will no longer depend on #114928.

joker-eph · 2024-11-08T12:59:39Z

mlir/include/mlir/IR/Builders.h

@@ -334,6 +334,18 @@ class OpBuilder : public Builder {
  /// This class represents a saved insertion point.
  class InsertPoint {
  public:
+    /// Finds the closest insertion point where all given values are defined


It's not clear to me that this is a well defined problem: what does "closest means" here?

Possible candidates are the positions (Block::iterator) right after the definition of each value. Out of these, there should be only one unique insertion point that post-dominates all the others.

joker-eph · 2024-11-08T13:00:34Z

mlir/lib/IR/Builders.cpp

+    }
+
+    // `pt` and `result` are in different subtrees: neither is an ancestor of
+    // the other. In that case, there is no suitable insertion point.


Wouldn't the right insertion point be the post-dominator block?

Yes. Post-dominator Block::iterator to be precise. (Adding support for that in #115587.)

…ation The implementations of `DominanceInfo::properlyDominates` and `PostDominanceInfo::properlyPostDominates` are almost identical: only one line of code is different. Define the function in `DominanceInfoBase` to avoid the code duplication. Note: This commit is not marked as NFC because `PostDominanceInfo::properlyPostDominates` now also has an `enclosingOpOk` argument.

The `properlyDominates` implementations for blocks and ops are very similar. This commit replaces them with a single implementation that operates on block iterators. That implementation can be used to implement both `properlyDominates` variants. Note: A subsequent commit will add a new public `properlyDominates` overload that accepts block iterators. That functionality can then be used to find a valid insertion point at which a range of values is defined (by utilizing post dominance). Depends on #115433.

matthias-springer · 2024-11-12T05:11:58Z

mlir/include/mlir/IR/Builders.h

+    /// There may be multiple suitable insertion points. This function chooses
+    /// an insertion right after one of the given values.
+    ///
+    /// Note: Some of the given values may already have gone out of scope at the


Note: if there exists an insertion point that post-dominates all definitions and where all values are in scope, this function is guaranteed to find it.

River707 · 2024-11-12T07:02:06Z

Why don't the various transforms just use DominanceInfo? Answering these queries is what it's intended for.

Block reachability and block dominance are two different things. Just because there is a path from block A -> block B does not necessarily mean that block A dominates block B. I'm not sure how "block reachability" could be implemented with DominanceInfo.

That being said, in the dialect conversion I actually need dominance, so what I implemented here is not useful there...

Yes, my comment was geared towards the fact that several of the usages are already conflating reachability with dominance (for example the original usage in the followup IRBuilder PR). My concern is/was around the general utility of such a function, given that it cannot be used in most situations people would reach for it (i.e. dominance outside of purely structured control flow) so it wasn't clear the utility in adding it to Block. That being said I don't have an issue with this singular function, given that it's a tiny amount of graph traversal code. I would clearly mark in the comment that this function is not a replacement for dominance in any reasonable situation (given that a successor block can easily reach a predecessor in a situation such as a loop).

matthias-springer · 2024-11-13T05:26:30Z

Yes, my comment was geared towards the fact that several of the usages are already conflating reachability with dominance (for example the original usage in the followup IRBuilder PR). My concern is/was around the general utility of such a function, given that it cannot be used in most situations people would reach for it (i.e. dominance outside of purely structured control flow) so it wasn't clear the utility in adding it to Block. That being said I don't have an issue with this singular function, given that it's a tiny amount of graph traversal code. I would clearly mark in the comment that this function is not a replacement for dominance in any reasonable situation (given that a successor block can easily reach a predecessor in a situation such as a loop).

I think this comment is referring to #114928. I updated the comment on that PR.

matthias-springer · 2024-11-13T05:28:13Z

I don't have any immediate use case for this PR anymore.

matthias-springer requested review from jpienaar, joker-eph, zero9178 and River707 November 5, 2024 06:09

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 5, 2024

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from 4881106 to 08be950 Compare November 5, 2024 06:12

River707 reviewed Nov 5, 2024

View reviewed changes

mlir/lib/IR/Builders.cpp Outdated Show resolved Hide resolved

River707 mentioned this pull request Nov 5, 2024

[mlir][IR] Add Block::isReachable helper function #114928

Merged

matthias-springer marked this pull request as draft November 8, 2024 07:42

joker-eph reviewed Nov 8, 2024

View reviewed changes

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from 08be950 to a18a6c0 Compare November 9, 2024 11:30

matthias-springer changed the base branch from users/matthias-springer/block_is_reachable to users/matthias-springer/post_dom_constant November 9, 2024 11:30

matthias-springer changed the title ~~[mlir][IR] Add helper functions to compute insertion point~~ [mlir][IR] Add OpBuilder::setInsertionPointAfterValues Nov 9, 2024

matthias-springer force-pushed the users/matthias-springer/post_dom_constant branch from caea72f to e890126 Compare November 10, 2024 06:24

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from a18a6c0 to f716eaa Compare November 10, 2024 06:25

matthias-springer force-pushed the users/matthias-springer/post_dom_constant branch from e890126 to 943bd95 Compare November 11, 2024 02:33

Base automatically changed from users/matthias-springer/post_dom_constant to main November 11, 2024 02:43

matthias-springer added 3 commits November 11, 2024 03:43

[mlir][IR][NFC] DominanceInfo: Minor code cleanups

8bc07a5

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from f716eaa to df99520 Compare November 11, 2024 02:50

matthias-springer changed the base branch from main to users/matthias-springer/block_dom November 11, 2024 02:50

matthias-springer changed the title ~~[mlir][IR] Add OpBuilder::setInsertionPointAfterValues~~ [mlir][IR] Add InsertPoint::after(ValueRange) Nov 12, 2024

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from df99520 to f1dbd8a Compare November 12, 2024 04:44

[mlir][IR] Add OpBuilder::setInsertionPointAfterValues

8a49434

matthias-springer force-pushed the users/matthias-springer/insert_pt branch from f1dbd8a to 8a49434 Compare November 12, 2024 04:44

matthias-springer mentioned this pull request Nov 12, 2024

[mlir][Transforms] Dialect Conversion: Add replaceOpWithMultiple #115816

Merged

matthias-springer marked this pull request as ready for review November 12, 2024 05:09

matthias-springer requested review from River707 and joker-eph November 12, 2024 05:09

matthias-springer commented Nov 12, 2024

View reviewed changes

matthias-springer force-pushed the users/matthias-springer/block_dom branch from 57bd391 to c56dc33 Compare November 12, 2024 10:59

matthias-springer closed this Nov 13, 2024

This was referenced Dec 20, 2024

[mlir][Transforms] Support 1:N mappings in ConversionValueMapping #116524

Merged

[mlir][Transforms][NFC] Use DominanceInfo to compute materialization insertion point #120746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][IR] Add `InsertPoint::after(ValueRange)` #114940

[mlir][IR] Add `InsertPoint::after(ValueRange)` #114940

Uh oh!

matthias-springer commented Nov 5, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

matthias-springer commented Nov 5, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

matthias-springer commented Nov 8, 2024 •

edited

Loading

Uh oh!

joker-eph Nov 8, 2024

Uh oh!

matthias-springer Nov 9, 2024 •

edited

Loading

Uh oh!

joker-eph Nov 8, 2024

Uh oh!

matthias-springer Nov 9, 2024

Uh oh!

matthias-springer Nov 12, 2024

Uh oh!

River707 commented Nov 12, 2024

Uh oh!

matthias-springer commented Nov 13, 2024

Uh oh!

matthias-springer commented Nov 13, 2024

Uh oh!

Uh oh!

[mlir][IR] Add InsertPoint::after(ValueRange) #114940

[mlir][IR] Add InsertPoint::after(ValueRange) #114940

Uh oh!

Conversation

matthias-springer commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

matthias-springer commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matthias-springer commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

matthias-springer Nov 9, 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!

joker-eph Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

matthias-springer Nov 9, 2024

Choose a reason for hiding this comment

Uh oh!

matthias-springer Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

River707 commented Nov 12, 2024

Uh oh!

matthias-springer commented Nov 13, 2024

Uh oh!

matthias-springer commented Nov 13, 2024

Uh oh!

Uh oh!

[mlir][IR] Add `InsertPoint::after(ValueRange)` #114940

[mlir][IR] Add `InsertPoint::after(ValueRange)` #114940

matthias-springer commented Nov 5, 2024 •

edited

Loading

matthias-springer commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading

matthias-springer commented Nov 8, 2024 •

edited

Loading

matthias-springer Nov 9, 2024 •

edited

Loading