[MLIR] Erase location of folded constants #75415
Conversation
zyx-billy
force-pushed
the
mlir/erase_folded_location
branch
from
24954d5 to
de9e7e3
Compare
zyx-billy
force-pushed
the
mlir/erase_folded_location
branch
from
de9e7e3 to
d9045e4
Compare
|
@llvm/pr-subscribers-mlir-core Author: Billy Zhu (zyx-billy) ChangesFollow up to the discussion from #75258, and serves as an alternate solution for #74670. Set the location to Unknown for deduplicated / moved / materialized constants by OperationFolder. This makes sure that the folded constants don't end up with an arbitrary location of one of the original ops that became it, and that hoisted ops don't confuse the stepping order. One foreseeable side effect is that since the canonicalizer always hoists / materializes constants to the front, any IR going through the canonicalizer will have its constants' locations erased. If it makes a difference, we can consider making OperationFolder more lazy by only hoisting when deduplicating to partially avoid this issue, though I'm not sure how much the existing hoisting behavior is already relied on (it broke quite a lot of mlir tests). Full diff: https://github.com/llvm/llvm-project/pull/75415.diff 7 Files Affected:
diff --git a/mlir/include/mlir/Transforms/FoldUtils.h b/mlir/include/mlir/Transforms/FoldUtils.h
index 2600da361496cd..2e7a6fe3e362cb 100644
--- a/mlir/include/mlir/Transforms/FoldUtils.h
+++ b/mlir/include/mlir/Transforms/FoldUtils.h
@@ -33,7 +33,8 @@ class Value;
class OperationFolder {
public:
OperationFolder(MLIRContext *ctx, OpBuilder::Listener *listener = nullptr)
- : interfaces(ctx), rewriter(ctx, listener) {}
+ : erasedFoldedLocation(UnknownLoc::get(ctx)), interfaces(ctx),
+ rewriter(ctx, listener) {}
/// Tries to perform folding on the given `op`, including unifying
/// deduplicated constants. If successful, replaces `op`'s uses with
@@ -65,7 +66,7 @@ class OperationFolder {
/// be created in a parent block. On success this returns the constant
/// operation, nullptr otherwise.
Value getOrCreateConstant(Block *block, Dialect *dialect, Attribute value,
- Type type, Location loc);
+ Type type);
private:
/// This map keeps track of uniqued constants by dialect, attribute, and type.
@@ -95,6 +96,9 @@ class OperationFolder {
Dialect *dialect, Attribute value,
Type type, Location loc);
+ /// The location to overwrite with for folder-owned constants.
+ UnknownLoc erasedFoldedLocation;
+
/// A mapping between an insertion region and the constants that have been
/// created within it.
DenseMap<Region *, ConstantMap> foldScopes;
diff --git a/mlir/lib/Transforms/SCCP.cpp b/mlir/lib/Transforms/SCCP.cpp
index 14435b37acc914..b2d3929b045968 100644
--- a/mlir/lib/Transforms/SCCP.cpp
+++ b/mlir/lib/Transforms/SCCP.cpp
@@ -53,7 +53,7 @@ static LogicalResult replaceWithConstant(DataFlowSolver &solver,
Dialect *dialect = latticeValue.getConstantDialect();
Value constant = folder.getOrCreateConstant(
builder.getInsertionBlock(), dialect, latticeValue.getConstantValue(),
- value.getType(), value.getLoc());
+ value.getType());
if (!constant)
return failure();
diff --git a/mlir/lib/Transforms/Utils/FoldUtils.cpp b/mlir/lib/Transforms/Utils/FoldUtils.cpp
index 90ee5ba51de3ad..8221c12fd6b2dd 100644
--- a/mlir/lib/Transforms/Utils/FoldUtils.cpp
+++ b/mlir/lib/Transforms/Utils/FoldUtils.cpp
@@ -154,6 +154,7 @@ bool OperationFolder::insertKnownConstant(Operation *op, Attribute constValue) {
!isFolderOwnedConstant(op->getPrevNode())))
op->moveBefore(&insertBlock->front());
+ op->setLoc(erasedFoldedLocation);
folderConstOp = op;
referencedDialects[op].push_back(op->getDialect());
return true;
@@ -193,8 +194,7 @@ void OperationFolder::clear() {
/// Get or create a constant using the given builder. On success this returns
/// the constant operation, nullptr otherwise.
Value OperationFolder::getOrCreateConstant(Block *block, Dialect *dialect,
- Attribute value, Type type,
- Location loc) {
+ Attribute value, Type type) {
// Find an insertion point for the constant.
auto *insertRegion = getInsertionRegion(interfaces, block);
auto &entry = insertRegion->front();
@@ -202,8 +202,8 @@ Value OperationFolder::getOrCreateConstant(Block *block, Dialect *dialect,
// Get the constant map for the insertion region of this operation.
auto &uniquedConstants = foldScopes[insertRegion];
- Operation *constOp =
- tryGetOrCreateConstant(uniquedConstants, dialect, value, type, loc);
+ Operation *constOp = tryGetOrCreateConstant(uniquedConstants, dialect, value,
+ type, erasedFoldedLocation);
return constOp ? constOp->getResult(0) : Value();
}
@@ -258,8 +258,9 @@ OperationFolder::processFoldResults(Operation *op,
// Check to see if there is a canonicalized version of this constant.
auto res = op->getResult(i);
Attribute attrRepl = foldResults[i].get<Attribute>();
- if (auto *constOp = tryGetOrCreateConstant(
- uniquedConstants, dialect, attrRepl, res.getType(), op->getLoc())) {
+ if (auto *constOp =
+ tryGetOrCreateConstant(uniquedConstants, dialect, attrRepl,
+ res.getType(), erasedFoldedLocation)) {
// Ensure that this constant dominates the operation we are replacing it
// with. This may not automatically happen if the operation being folded
// was inserted before the constant within the insertion block.
diff --git a/mlir/test/Dialect/Transform/test-pattern-application.mlir b/mlir/test/Dialect/Transform/test-pattern-application.mlir
index 2fd47c6bae396a..ff9a535c838432 100644
--- a/mlir/test/Dialect/Transform/test-pattern-application.mlir
+++ b/mlir/test/Dialect/Transform/test-pattern-application.mlir
@@ -179,7 +179,6 @@ module {
// CHECK: return %[[c5]]
func.func @canonicalization(%t: tensor<5xf32>) -> index {
%c0 = arith.constant 0 : index
- // expected-remark @below {{op was replaced}}
%dim = tensor.dim %t, %c0 : tensor<5xf32>
return %dim : index
}
@@ -191,7 +190,6 @@ transform.sequence failures(propagate) {
transform.apply_patterns to %1 {
transform.apply_patterns.canonicalization
} : !transform.any_op
- transform.test_print_remark_at_operand %0, "op was replaced" : !transform.any_op
}
// -----
diff --git a/mlir/test/Transforms/canonicalize-debuginfo.mlir b/mlir/test/Transforms/canonicalize-debuginfo.mlir
new file mode 100644
index 00000000000000..b80da2bbdbba70
--- /dev/null
+++ b/mlir/test/Transforms/canonicalize-debuginfo.mlir
@@ -0,0 +1,42 @@
+// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' -split-input-file -mlir-print-debuginfo | FileCheck %s
+
+// CHECK-LABEL: func @merge_constants
+func.func @merge_constants() -> (index, index, index, index) {
+ // CHECK-NEXT: arith.constant 42 : index loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 42 : index loc("merge_constants":0:0)
+ %1 = arith.constant 42 : index loc("merge_constants":1:0)
+ %2 = arith.constant 42 : index loc("merge_constants":2:0)
+ %3 = arith.constant 42 : index loc("merge_constants":2:0)
+ return %0, %1, %2, %3 : index, index, index, index
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @simple_hoist
+func.func @simple_hoist(%arg0: memref<8xi32>) -> i32 {
+ // CHECK: arith.constant 88 : i32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 42 : i32 loc("simple_hoist":0:0)
+ %1 = arith.constant 0 : index loc("simple_hoist":1:0)
+ memref.store %0, %arg0[%1] : memref<8xi32>
+
+ %2 = arith.constant 88 : i32 loc("simple_hoist":2:0)
+
+ return %2 : i32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @hoist_and_merge
+func.func @hoist_and_merge(%arg0: memref<8xi32>) {
+ // CHECK-NEXT: arith.constant 42 : i32 loc(#[[UnknownLoc:.*]])
+ affine.for %arg1 = 0 to 8 {
+ %0 = arith.constant 42 : i32 loc("hoist_and_merge":0:0)
+ %1 = arith.constant 42 : i32 loc("hoist_and_merge":1:0)
+ memref.store %0, %arg0[%arg1] : memref<8xi32>
+ memref.store %1, %arg0[%arg1] : memref<8xi32>
+ }
+ return
+} loc("hoist_and_merge":2:0)
+// CHECK: #[[UnknownLoc]] = loc(unknown)
diff --git a/mlir/test/Transforms/constant-fold-debuginfo.mlir b/mlir/test/Transforms/constant-fold-debuginfo.mlir
new file mode 100644
index 00000000000000..c308bc477bee44
--- /dev/null
+++ b/mlir/test/Transforms/constant-fold-debuginfo.mlir
@@ -0,0 +1,42 @@
+// RUN: mlir-opt %s -split-input-file -test-constant-fold -mlir-print-debuginfo | FileCheck %s
+
+// CHECK-LABEL: func @fold_and_merge
+func.func @fold_and_merge() -> (i32, i32) {
+ // CHECK-NEXT: [[C:%.+]] = arith.constant 6 : i32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 1 : i32 loc("fold_and_merge":0:0)
+ %1 = arith.constant 5 : i32 loc("fold_and_merge":1:0)
+ %2 = arith.addi %0, %1 : i32 loc("fold_and_merge":2:0)
+
+ %3 = arith.constant 6 : i32 loc("fold_and_merge":3:0)
+
+ return %2, %3: i32, i32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @materialize_different_dialect
+func.func @materialize_different_dialect() -> (f32, f32) {
+ // CHECK: arith.constant 1.{{0*}}e+00 : f32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant -1.0 : f32 loc("materialize_different_dialect":0:0)
+ %1 = math.absf %0 : f32 loc("materialize_different_dialect":1:0)
+ %2 = arith.constant 1.0 : f32 loc("materialize_different_dialect":2:0)
+
+ return %1, %2: f32, f32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @materialize_in_front
+func.func @materialize_in_front(%arg0: memref<8xi32>) {
+ // CHECK-NEXT: arith.constant 6 : i32 loc(#[[UnknownLoc:.*]])
+ affine.for %arg1 = 0 to 8 {
+ %1 = arith.constant 1 : i32 loc("materialize_in_front":0:0)
+ %2 = arith.constant 5 : i32 loc("materialize_in_front":1:0)
+ %3 = arith.addi %1, %2 : i32 loc("materialize_in_front":2:0)
+ memref.store %3, %arg0[%arg1] : memref<8xi32>
+ }
+ return
+} loc("materialize_in_front":3:0)
+// CHECK: #[[UnknownLoc]] = loc(unknown)
diff --git a/mlir/test/lib/Transforms/TestIntRangeInference.cpp b/mlir/test/lib/Transforms/TestIntRangeInference.cpp
index 2f6dd5b8095dfa..5758f6acf2f0ff 100644
--- a/mlir/test/lib/Transforms/TestIntRangeInference.cpp
+++ b/mlir/test/lib/Transforms/TestIntRangeInference.cpp
@@ -40,9 +40,8 @@ static LogicalResult replaceWithConstant(DataFlowSolver &solver, OpBuilder &b,
maybeDefiningOp ? maybeDefiningOp->getDialect()
: value.getParentRegion()->getParentOp()->getDialect();
Attribute constAttr = b.getIntegerAttr(value.getType(), *maybeConstValue);
- Value constant =
- folder.getOrCreateConstant(b.getInsertionBlock(), valueDialect, constAttr,
- value.getType(), value.getLoc());
+ Value constant = folder.getOrCreateConstant(
+ b.getInsertionBlock(), valueDialect, constAttr, value.getType());
if (!constant)
return failure();
|
|
@llvm/pr-subscribers-mlir Author: Billy Zhu (zyx-billy) ChangesFollow up to the discussion from #75258, and serves as an alternate solution for #74670. Set the location to Unknown for deduplicated / moved / materialized constants by OperationFolder. This makes sure that the folded constants don't end up with an arbitrary location of one of the original ops that became it, and that hoisted ops don't confuse the stepping order. One foreseeable side effect is that since the canonicalizer always hoists / materializes constants to the front, any IR going through the canonicalizer will have its constants' locations erased. If it makes a difference, we can consider making OperationFolder more lazy by only hoisting when deduplicating to partially avoid this issue, though I'm not sure how much the existing hoisting behavior is already relied on (it broke quite a lot of mlir tests). Full diff: https://github.com/llvm/llvm-project/pull/75415.diff 7 Files Affected:
diff --git a/mlir/include/mlir/Transforms/FoldUtils.h b/mlir/include/mlir/Transforms/FoldUtils.h
index 2600da361496cd..2e7a6fe3e362cb 100644
--- a/mlir/include/mlir/Transforms/FoldUtils.h
+++ b/mlir/include/mlir/Transforms/FoldUtils.h
@@ -33,7 +33,8 @@ class Value;
class OperationFolder {
public:
OperationFolder(MLIRContext *ctx, OpBuilder::Listener *listener = nullptr)
- : interfaces(ctx), rewriter(ctx, listener) {}
+ : erasedFoldedLocation(UnknownLoc::get(ctx)), interfaces(ctx),
+ rewriter(ctx, listener) {}
/// Tries to perform folding on the given `op`, including unifying
/// deduplicated constants. If successful, replaces `op`'s uses with
@@ -65,7 +66,7 @@ class OperationFolder {
/// be created in a parent block. On success this returns the constant
/// operation, nullptr otherwise.
Value getOrCreateConstant(Block *block, Dialect *dialect, Attribute value,
- Type type, Location loc);
+ Type type);
private:
/// This map keeps track of uniqued constants by dialect, attribute, and type.
@@ -95,6 +96,9 @@ class OperationFolder {
Dialect *dialect, Attribute value,
Type type, Location loc);
+ /// The location to overwrite with for folder-owned constants.
+ UnknownLoc erasedFoldedLocation;
+
/// A mapping between an insertion region and the constants that have been
/// created within it.
DenseMap<Region *, ConstantMap> foldScopes;
diff --git a/mlir/lib/Transforms/SCCP.cpp b/mlir/lib/Transforms/SCCP.cpp
index 14435b37acc914..b2d3929b045968 100644
--- a/mlir/lib/Transforms/SCCP.cpp
+++ b/mlir/lib/Transforms/SCCP.cpp
@@ -53,7 +53,7 @@ static LogicalResult replaceWithConstant(DataFlowSolver &solver,
Dialect *dialect = latticeValue.getConstantDialect();
Value constant = folder.getOrCreateConstant(
builder.getInsertionBlock(), dialect, latticeValue.getConstantValue(),
- value.getType(), value.getLoc());
+ value.getType());
if (!constant)
return failure();
diff --git a/mlir/lib/Transforms/Utils/FoldUtils.cpp b/mlir/lib/Transforms/Utils/FoldUtils.cpp
index 90ee5ba51de3ad..8221c12fd6b2dd 100644
--- a/mlir/lib/Transforms/Utils/FoldUtils.cpp
+++ b/mlir/lib/Transforms/Utils/FoldUtils.cpp
@@ -154,6 +154,7 @@ bool OperationFolder::insertKnownConstant(Operation *op, Attribute constValue) {
!isFolderOwnedConstant(op->getPrevNode())))
op->moveBefore(&insertBlock->front());
+ op->setLoc(erasedFoldedLocation);
folderConstOp = op;
referencedDialects[op].push_back(op->getDialect());
return true;
@@ -193,8 +194,7 @@ void OperationFolder::clear() {
/// Get or create a constant using the given builder. On success this returns
/// the constant operation, nullptr otherwise.
Value OperationFolder::getOrCreateConstant(Block *block, Dialect *dialect,
- Attribute value, Type type,
- Location loc) {
+ Attribute value, Type type) {
// Find an insertion point for the constant.
auto *insertRegion = getInsertionRegion(interfaces, block);
auto &entry = insertRegion->front();
@@ -202,8 +202,8 @@ Value OperationFolder::getOrCreateConstant(Block *block, Dialect *dialect,
// Get the constant map for the insertion region of this operation.
auto &uniquedConstants = foldScopes[insertRegion];
- Operation *constOp =
- tryGetOrCreateConstant(uniquedConstants, dialect, value, type, loc);
+ Operation *constOp = tryGetOrCreateConstant(uniquedConstants, dialect, value,
+ type, erasedFoldedLocation);
return constOp ? constOp->getResult(0) : Value();
}
@@ -258,8 +258,9 @@ OperationFolder::processFoldResults(Operation *op,
// Check to see if there is a canonicalized version of this constant.
auto res = op->getResult(i);
Attribute attrRepl = foldResults[i].get<Attribute>();
- if (auto *constOp = tryGetOrCreateConstant(
- uniquedConstants, dialect, attrRepl, res.getType(), op->getLoc())) {
+ if (auto *constOp =
+ tryGetOrCreateConstant(uniquedConstants, dialect, attrRepl,
+ res.getType(), erasedFoldedLocation)) {
// Ensure that this constant dominates the operation we are replacing it
// with. This may not automatically happen if the operation being folded
// was inserted before the constant within the insertion block.
diff --git a/mlir/test/Dialect/Transform/test-pattern-application.mlir b/mlir/test/Dialect/Transform/test-pattern-application.mlir
index 2fd47c6bae396a..ff9a535c838432 100644
--- a/mlir/test/Dialect/Transform/test-pattern-application.mlir
+++ b/mlir/test/Dialect/Transform/test-pattern-application.mlir
@@ -179,7 +179,6 @@ module {
// CHECK: return %[[c5]]
func.func @canonicalization(%t: tensor<5xf32>) -> index {
%c0 = arith.constant 0 : index
- // expected-remark @below {{op was replaced}}
%dim = tensor.dim %t, %c0 : tensor<5xf32>
return %dim : index
}
@@ -191,7 +190,6 @@ transform.sequence failures(propagate) {
transform.apply_patterns to %1 {
transform.apply_patterns.canonicalization
} : !transform.any_op
- transform.test_print_remark_at_operand %0, "op was replaced" : !transform.any_op
}
// -----
diff --git a/mlir/test/Transforms/canonicalize-debuginfo.mlir b/mlir/test/Transforms/canonicalize-debuginfo.mlir
new file mode 100644
index 00000000000000..b80da2bbdbba70
--- /dev/null
+++ b/mlir/test/Transforms/canonicalize-debuginfo.mlir
@@ -0,0 +1,42 @@
+// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize{test-convergence}))' -split-input-file -mlir-print-debuginfo | FileCheck %s
+
+// CHECK-LABEL: func @merge_constants
+func.func @merge_constants() -> (index, index, index, index) {
+ // CHECK-NEXT: arith.constant 42 : index loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 42 : index loc("merge_constants":0:0)
+ %1 = arith.constant 42 : index loc("merge_constants":1:0)
+ %2 = arith.constant 42 : index loc("merge_constants":2:0)
+ %3 = arith.constant 42 : index loc("merge_constants":2:0)
+ return %0, %1, %2, %3 : index, index, index, index
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @simple_hoist
+func.func @simple_hoist(%arg0: memref<8xi32>) -> i32 {
+ // CHECK: arith.constant 88 : i32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 42 : i32 loc("simple_hoist":0:0)
+ %1 = arith.constant 0 : index loc("simple_hoist":1:0)
+ memref.store %0, %arg0[%1] : memref<8xi32>
+
+ %2 = arith.constant 88 : i32 loc("simple_hoist":2:0)
+
+ return %2 : i32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @hoist_and_merge
+func.func @hoist_and_merge(%arg0: memref<8xi32>) {
+ // CHECK-NEXT: arith.constant 42 : i32 loc(#[[UnknownLoc:.*]])
+ affine.for %arg1 = 0 to 8 {
+ %0 = arith.constant 42 : i32 loc("hoist_and_merge":0:0)
+ %1 = arith.constant 42 : i32 loc("hoist_and_merge":1:0)
+ memref.store %0, %arg0[%arg1] : memref<8xi32>
+ memref.store %1, %arg0[%arg1] : memref<8xi32>
+ }
+ return
+} loc("hoist_and_merge":2:0)
+// CHECK: #[[UnknownLoc]] = loc(unknown)
diff --git a/mlir/test/Transforms/constant-fold-debuginfo.mlir b/mlir/test/Transforms/constant-fold-debuginfo.mlir
new file mode 100644
index 00000000000000..c308bc477bee44
--- /dev/null
+++ b/mlir/test/Transforms/constant-fold-debuginfo.mlir
@@ -0,0 +1,42 @@
+// RUN: mlir-opt %s -split-input-file -test-constant-fold -mlir-print-debuginfo | FileCheck %s
+
+// CHECK-LABEL: func @fold_and_merge
+func.func @fold_and_merge() -> (i32, i32) {
+ // CHECK-NEXT: [[C:%.+]] = arith.constant 6 : i32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant 1 : i32 loc("fold_and_merge":0:0)
+ %1 = arith.constant 5 : i32 loc("fold_and_merge":1:0)
+ %2 = arith.addi %0, %1 : i32 loc("fold_and_merge":2:0)
+
+ %3 = arith.constant 6 : i32 loc("fold_and_merge":3:0)
+
+ return %2, %3: i32, i32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @materialize_different_dialect
+func.func @materialize_different_dialect() -> (f32, f32) {
+ // CHECK: arith.constant 1.{{0*}}e+00 : f32 loc(#[[UnknownLoc:.*]])
+ %0 = arith.constant -1.0 : f32 loc("materialize_different_dialect":0:0)
+ %1 = math.absf %0 : f32 loc("materialize_different_dialect":1:0)
+ %2 = arith.constant 1.0 : f32 loc("materialize_different_dialect":2:0)
+
+ return %1, %2: f32, f32
+}
+// CHECK: #[[UnknownLoc]] = loc(unknown)
+
+// -----
+
+// CHECK-LABEL: func @materialize_in_front
+func.func @materialize_in_front(%arg0: memref<8xi32>) {
+ // CHECK-NEXT: arith.constant 6 : i32 loc(#[[UnknownLoc:.*]])
+ affine.for %arg1 = 0 to 8 {
+ %1 = arith.constant 1 : i32 loc("materialize_in_front":0:0)
+ %2 = arith.constant 5 : i32 loc("materialize_in_front":1:0)
+ %3 = arith.addi %1, %2 : i32 loc("materialize_in_front":2:0)
+ memref.store %3, %arg0[%arg1] : memref<8xi32>
+ }
+ return
+} loc("materialize_in_front":3:0)
+// CHECK: #[[UnknownLoc]] = loc(unknown)
diff --git a/mlir/test/lib/Transforms/TestIntRangeInference.cpp b/mlir/test/lib/Transforms/TestIntRangeInference.cpp
index 2f6dd5b8095dfa..5758f6acf2f0ff 100644
--- a/mlir/test/lib/Transforms/TestIntRangeInference.cpp
+++ b/mlir/test/lib/Transforms/TestIntRangeInference.cpp
@@ -40,9 +40,8 @@ static LogicalResult replaceWithConstant(DataFlowSolver &solver, OpBuilder &b,
maybeDefiningOp ? maybeDefiningOp->getDialect()
: value.getParentRegion()->getParentOp()->getDialect();
Attribute constAttr = b.getIntegerAttr(value.getType(), *maybeConstValue);
- Value constant =
- folder.getOrCreateConstant(b.getInsertionBlock(), valueDialect, constAttr,
- value.getType(), value.getLoc());
+ Value constant = folder.getOrCreateConstant(
+ b.getInsertionBlock(), valueDialect, constAttr, value.getType());
if (!constant)
return failure();
|
| @@ -154,6 +154,7 @@ bool OperationFolder::insertKnownConstant(Operation *op, Attribute constValue) { | |||
| !isFolderOwnedConstant(op->getPrevNode()))) | |||
| op->moveBefore(&insertBlock->front()); | |||
|
|
|||
| op->setLoc(erasedFoldedLocation); | |||
There was a problem hiding this comment.
I would think we'd only need to erase the location if we actually move the operation? (that is: the check 3 lines above)
There was a problem hiding this comment.
oh yes it comes down to two policies:
- Erase location when the folder "owns" it, or
- Erase location when the folder manipulates it (hoist it or deduplicate something else with it)
Since there's only one place where the folder "ingests" an existing op to own it, I thought tracking it early would make the logic simpler to maintain. I don't have a really strong preference though. Let me push a patch on top to erase late and we can choose. The benefit with the latter of course is for constants that are already at the top of a block, they're unaffected.
There was a problem hiding this comment.
The benefit with the latter of course is for constants that are already at the top of a block, they're unaffected.
Yes that is what I had in mind: the folder taking "ownership" of a constant, but this one never being moved.
| // Check if an existing mapping already exists. | ||
| auto constKey = std::make_tuple(dialect, value, type); | ||
| Operation *&constOp = uniquedConstants[constKey]; | ||
| if (constOp) | ||
| if (constOp) { | ||
| constOp->setLoc(erasedFoldedLocation); |
There was a problem hiding this comment.
This one isn't clear to me: why do we need to erase here?
There was a problem hiding this comment.
Oh this is because when people call tryGetOrCreateConstant, they expect to be getting an existing constant or materializing a new constant for use elsewhere as a result of a fold.
Say the operation folder originally owned a constant op for index 6. Then somewhere else in the same block, the folder folded (5 + 1) into 6. Now it will call this function to see if we already have a 6. Since it does, the original constant 6 op gets returned to the caller. The caller will use this 6 op to replace the folded op. This means the original 6 op is now deduplicated with the folded op, so will need an erased location. (also see test case fold_and_merge below).
More generally, the new contract of this function says that since we know that the caller of this function is going to take whatever constant we return and use somewhere else, the location of this returned op should be erased since it's now a shared constant. The caller should not have to care if this op is reused or materialized, so both cases should return an erased location.
| @@ -318,6 +327,7 @@ OperationFolder::tryGetOrCreateConstant(ConstantMap &uniquedConstants, | |||
| notifyRemoval(constOp); | |||
| rewriter.eraseOp(constOp); | |||
| referencedDialects[existingOp].push_back(dialect); | |||
| existingOp->setLoc(erasedFoldedLocation); | |||
There was a problem hiding this comment.
Same logic applies here (see test case materialize_different_dialect below).
Though now I think we can make this a bit more explicit by still having the user pass in a location, but if the new location doesn't match the existing location, then we erase it. That way it's more clear what's going on.
There was a problem hiding this comment.
Actually, in practice it doesn't really make a difference since the only two callers of this private function tryGetOrCreateConstant already set the insertion point to the beginning of a block before calling it, so they should already know to ask for an erased location otherwise it's not going to work.
|
Since locations are used for more than just line numbers and lead to other kind of context than debugging, this may bite us and we may have to revet this in the future. Thinks of DL graphs where you rather keep a constant name for the node instead of erasing it when it becomes shared. We can give it a try though. |
|
Thank you @joker-eph! Yes I agree the uses are a bit fuzzy today. If we end up breaking many existing use cases, I'm happy to revert it, though that'll be a good time for MLIR as a whole to spell out exactly what locations are meant for (is it somewhere already?). Since in order for dialect-independent passes to do anything involving locations, we first need dialect-independent semantics for locations. It's possible there are already conflicting dialect-defined location semantics but things never broke too much for folks to go in and enforce it. |
|
Honestly this feel weird: whenever I see an unknown loc in a module I think somebody messed up and didn't do a good job tracking. I see unknown locations as bugs, where information was lost. |
@jpienaar I agree with the sentiment... Today we're jumping between two extremes: tracking everything & tracking nothing. Our first approach was to track everything, which lead to the OOM error and had to be reverted, so we went with tracking nothing instead. One middle ground I see is to mark deduplicated constants with the common parent region's location (similar in spirit to what llvm does when merging locations. Without the ability to just fuse a bunch of locations like mlir, it had to create a single "common" location). If this seems reasonable, I'm happy to make this change. I'm about to submit a similar patch for CSE too, so it'd be great to solidify the approach. At the end of the day, I don't feel like we have much of a choice when two ops are merged... We either track both locations, track a common parent, or track nothing (there may be different ways to encode this, of course). In contrast, the previous approach was to just track an arbitrary one, so I do think we're moving in the right direction. Of course please LMK if you have any alternative suggestions 🙏. |
Follow up to the discussion from #75258, and serves as an alternate solution for #74670.
Set the location to Unknown for deduplicated / moved / materialized constants by OperationFolder. This makes sure that the folded constants don't end up with an arbitrary location of one of the original ops that became it, and that hoisted ops don't confuse the stepping order.
One foreseeable side effect is that since the canonicalizer always hoists / materializes constants to the front, any IR going through the canonicalizer will have its constants' locations erased. If it makes a difference, we can consider making OperationFolder more lazy by only hoisting when deduplicating to partially avoid this issue, though I'm not sure how much the existing hoisting behavior is already relied on (it broke quite a lot of mlir tests).