[Mlir-commits] [mlir] 8dbddb1 - [mlir] allow region branch spec from parent op to itself
Alex Zinenko
llvmlistbot at llvm.org
Fri Jul 21 02:17:02 PDT 2023
Author: Alex Zinenko
Date: 2023-07-21T09:16:56Z
New Revision: 8dbddb17180fff0ed881d75689651769b9a9b483
URL: https://github.com/llvm/llvm-project/commit/8dbddb17180fff0ed881d75689651769b9a9b483
DIFF: https://github.com/llvm/llvm-project/commit/8dbddb17180fff0ed881d75689651769b9a9b483.diff
LOG: [mlir] allow region branch spec from parent op to itself
RegionBranchOpInterface did not allow the operation with regions to
specify itself as successors. Therefore, this implied that the control
is always transferred to a region before being transferred back to the
parent op. Since the region can only transfer the control back to the
parent op from a terminator, this transitively implied that the first
block of any region with a RegionBranchOpInterface is always executed
until the terminator can transfer the control flow back. This is
trivially false for any conditional-like operation that may or may not
execute the region, as well as for loop-like operations that may not
execute the body.
Remove the restriction from the interface description and update the
only transform that relied on it.
See
https://discourse.llvm.org/t/rfc-region-control-flow-interfaces-should-encode-region-not-executed-correctly/72103.
Depends On: https://reviews.llvm.org/D155757
Reviewed By: Mogball, springerm
Differential Revision: https://reviews.llvm.org/D155822
Added:
Modified:
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
mlir/lib/Dialect/Bufferization/Transforms/BufferViewFlowAnalysis.cpp
mlir/lib/Dialect/SCF/IR/SCF.cpp
mlir/test/Analysis/DataFlow/test-next-access.mlir
Removed:
################################################################################
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index 67c668421238a1..1b75645261a8af 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -154,8 +154,9 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
either correspond to a constant value for each operand of this
operation, or null if that operand is not a constant. If `index` is
valid, `operands` corresponds to the entry values of the region at
- `index`. Only a region, i.e. a valid `index`, may use the parent
- operation as a successor. This method allows for describing which
+ `index`. The parent operation, i.e. a null `index`, may specify itself
+ as successor, which indicates that the control flow may not enter any
+ region at all. This method allows for describing which
regions may be executed when entering an operation, and which regions
are executed after having executed another region of the parent op. The
successor region must be non-empty.
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index 81f331fd6c2400..7c14ea75d22539 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -2868,10 +2868,13 @@ void AffineIfOp::getSuccessorRegions(
regions.reserve(2);
regions.push_back(
RegionSuccessor(&getThenRegion(), getThenRegion().getArguments()));
- // Don't consider the else region if it is empty.
- if (!getElseRegion().empty())
+ // If the "else" region is empty, branch bach into parent.
+ if (getElseRegion().empty()) {
+ regions.push_back(getResults());
+ } else {
regions.push_back(
RegionSuccessor(&getElseRegion(), getElseRegion().getArguments()));
+ }
return;
}
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/BufferViewFlowAnalysis.cpp b/mlir/lib/Dialect/Bufferization/Transforms/BufferViewFlowAnalysis.cpp
index d964f801668f99..f9c32d777a3bd8 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/BufferViewFlowAnalysis.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/BufferViewFlowAnalysis.cpp
@@ -93,11 +93,11 @@ void BufferViewFlowAnalysis::build(Operation *op) {
for (RegionSuccessor &entrySuccessor : entrySuccessors) {
// Wire the entry region's successor arguments with the initial
// successor inputs.
- assert(entrySuccessor.getSuccessor() &&
- "Invalid entry region without an attached successor region");
registerDependencies(
regionInterface.getSuccessorEntryOperands(
- entrySuccessor.getSuccessor()->getRegionNumber()),
+ entrySuccessor.isParent()
+ ? std::optional<unsigned>()
+ : entrySuccessor.getSuccessor()->getRegionNumber()),
entrySuccessor.getSuccessorInputs());
}
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index d8e0ba1c8cd6de..aaa5e39cd2f3d2 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -534,13 +534,8 @@ ForOp mlir::scf::getForInductionVarOwner(Value val) {
/// Return operands used when entering the region at 'index'. These operands
/// correspond to the loop iterator operands, i.e., those excluding the
-/// induction variable. LoopOp only has one region, so 0 is the only valid value
-/// for `index`.
+/// induction variable.
OperandRange ForOp::getSuccessorEntryOperands(std::optional<unsigned> index) {
- assert(index && *index == 0 && "invalid region index");
-
- // The initial operands map to the loop arguments after the induction
- // variable.
return getInitArgs();
}
@@ -552,15 +547,9 @@ OperandRange ForOp::getSuccessorEntryOperands(std::optional<unsigned> index) {
void ForOp::getSuccessorRegions(std::optional<unsigned> index,
ArrayRef<Attribute> operands,
SmallVectorImpl<RegionSuccessor> ®ions) {
- // If the predecessor is the ForOp, branch into the body using the iterator
- // arguments.
- if (!index) {
- regions.push_back(RegionSuccessor(&getLoopBody(), getRegionIterArgs()));
- return;
- }
-
- // Otherwise, the loop may branch back to itself or the parent operation.
- assert(*index == 0 && "expected loop region");
+ // Both the operation itself and the region may be branching into the body or
+ // back into the operation itself. It is possible for loop not to enter the
+ // body.
regions.push_back(RegionSuccessor(&getLoopBody(), getRegionIterArgs()));
regions.push_back(RegionSuccessor(getResults()));
}
@@ -1728,14 +1717,10 @@ void ForallOp::getCanonicalizationPatterns(RewritePatternSet &results,
void ForallOp::getSuccessorRegions(std::optional<unsigned> index,
ArrayRef<Attribute> operands,
SmallVectorImpl<RegionSuccessor> ®ions) {
- // If the predecessor is ForallOp, branch into the body with empty arguments.
- if (!index) {
- regions.push_back(RegionSuccessor(&getRegion()));
- return;
- }
-
- // Otherwise, the loop should branch back to the parent operation.
- assert(*index == 0 && "expected loop region");
+ // Both the operation itself and the region may be branching into the body or
+ // back into the operation itself. It is possible for loop not to enter the
+ // body.
+ regions.push_back(RegionSuccessor(&getRegion()));
regions.push_back(RegionSuccessor());
}
@@ -2031,9 +2016,12 @@ void IfOp::getSuccessorRegions(std::optional<unsigned> index,
} else {
// If the condition isn't constant, both regions may be executed.
regions.push_back(RegionSuccessor(&getThenRegion()));
- // If the else region does not exist, it is not a viable successor.
+ // If the else region does not exist, it is not a viable successor, so the
+ // control will go back to this operation instead.
if (elseRegion)
regions.push_back(RegionSuccessor(elseRegion));
+ else
+ regions.push_back(RegionSuccessor());
return;
}
@@ -3040,15 +3028,10 @@ void ParallelOp::getCanonicalizationPatterns(RewritePatternSet &results,
void ParallelOp::getSuccessorRegions(
std::optional<unsigned> index, ArrayRef<Attribute> operands,
SmallVectorImpl<RegionSuccessor> ®ions) {
- // If the predecessor is ParallelOp, branch into the body with empty
- // arguments.
- if (!index) {
- regions.push_back(RegionSuccessor(&getRegion()));
- return;
- }
-
- assert(*index == 0 && "expected loop region");
- // Otherwise, the loop should branch back to the parent operation.
+ // Both the operation itself and the region may be branching into the body or
+ // back into the operation itself. It is possible for loop not to enter the
+ // body.
+ regions.push_back(RegionSuccessor(&getRegion()));
regions.push_back(RegionSuccessor());
}
diff --git a/mlir/test/Analysis/DataFlow/test-next-access.mlir b/mlir/test/Analysis/DataFlow/test-next-access.mlir
index a029baf8988b41..313a75c171d01d 100644
--- a/mlir/test/Analysis/DataFlow/test-next-access.mlir
+++ b/mlir/test/Analysis/DataFlow/test-next-access.mlir
@@ -70,10 +70,7 @@ func.func @dead_branch(%arg0: memref<f32>, %arg1: f32) -> f32 {
func.func @loop(%arg0: memref<?xf32>, %arg1: f32, %arg2: index, %arg3: index, %arg4: index) -> f32 {
%c0 = arith.constant 0.0 : f32
// CHECK: name = "pre"
- // CHECK-SAME: next_access = {{\[}}["loop"], "unknown"]
- // The above is not entirely correct when the loop has 0 iterations, but
- // the region control flow specificaiton is currently incapable of
- // specifying that.
+ // CHECK-SAME: next_access = {{\[}}["outside", "loop"], "unknown"]
memref.load %arg0[%arg4] {name = "pre"} : memref<?xf32>
%l = scf.for %i = %arg2 to %arg3 step %arg4 iter_args(%ia = %c0) -> (f32) {
// CHECK: name = "loop"
@@ -90,10 +87,7 @@ func.func @loop(%arg0: memref<?xf32>, %arg1: f32, %arg2: index, %arg3: index, %a
// CHECK-LABEL: @conditional
func.func @conditional(%cond: i1, %arg0: memref<f32>) {
// CHECK: name = "pre"
- // CHECK-SAME: next_access = {{\[}}["then"]]
- // The above is not entirely correct when the condition is false, but
- // the region control flow specificaiton is currently incapable of
- // specifying that.
+ // CHECK-SAME: next_access = {{\[}}["post", "then"]]
memref.load %arg0[] {name = "pre"}: memref<f32>
scf.if %cond {
// CHECK: name = "then"
@@ -126,10 +120,7 @@ func.func @two_sided_conditional(%cond: i1, %arg0: memref<f32>) {
func.func @dead_conditional(%arg0: memref<f32>) {
%false = arith.constant 0 : i1
// CHECK: name = "pre"
- // CHECK-SAME: next_access = ["unknown"]
- // The above is not entirely correct when the condition is false, but
- // the region control flow specificaiton is currently incapable of
- // specifying that.
+ // CHECK-SAME: next_access = {{\[}}["post"]]
memref.load %arg0[] {name = "pre"}: memref<f32>
scf.if %false {
// CHECK: name = "then"
@@ -301,10 +292,7 @@ func.func @infinite_recursive_call(%arg0: memref<f32>) {
// CHECK-LABEL: @recursive_call
func.func @recursive_call(%arg0: memref<f32>, %cond: i1) {
// CHECK: name = "pre"
- // CHECK-SAME: next_access = {{\[}}["pre"]]
- // The above is not entirely correct when the condition is false, but
- // the region control flow specificaiton is currently incapable of
- // specifying that.
+ // CHECK-SAME: next_access = {{\[}}["post", "pre"]]
memref.load %arg0[] {name = "pre"} : memref<f32>
scf.if %cond {
func.call @recursive_call(%arg0, %cond) : (memref<f32>, i1) -> ()
More information about the Mlir-commits
mailing list