[mlir][transform] Overhaul RegionBranchOpInterface implementations.
#111408
Conversation
This PR adds the `ReturnLike` trait to `transform.yield`. This is required in the one-shot bufferization pass since the merging of llvm#110332, which analyses any `FunctionOpInterface` and expects them to have a `ReturnLike` terminator. Signed-off-by: Ingo Müller <ingomueller@google.com>
ingomueller-net
requested review from
ftynse and
nicolasvasilache
as code owners
|
@llvm/pr-subscribers-mlir Author: Ingo Müller (ingomueller-net) ChangesThis PR adds the Full diff: https://github.com/llvm/llvm-project/pull/111408.diff 1 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Transform/IR/TransformOps.td b/mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
index b946fc8875860b..d3933cad920a3f 100644
--- a/mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
+++ b/mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
@@ -1358,7 +1358,8 @@ def VerifyOp : TransformDialectOp<"verify",
}
def YieldOp : TransformDialectOp<"yield",
- [Terminator, DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
+ [Terminator, ReturnLike,
+ DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
let summary = "Yields operation handles from a transform IR region";
let description = [{
This terminator operation yields operation handles from regions of the
|
|
One could also argue that the one-shot bufferization is using the interface wrong: AFAIU, |
If we give `transform.yield` the `ReturnLike` trait, then the checks made by `RegionBranchOpInterface` behave differently. More concretely, the check that the operands and results passed across the control flow edges from the parent op to its regions and back are equal (or compatible). Which results are passed back from regions to the parent op depend on the terminator and/or whether it is `ReturnLike`. This commit radically changes the implementations of the `alternatives`, `foreach`, and `sequence` ops, which are the ops that use `yield` as their terminator. In fact, all of these ops only ever pass control from the parent op to the region and from there back to the parent op. In particular and unlike `scf.for`, `transform.foreach` does *not* pass control from one iteration of its body to the next directly; it rather passes the control back to the parent op, which then passes it back to the body for the next iteration. That can be seen by the fact that the body always gets arguments of the same type as the operands of the parent op (and none of the yielded types) and the types that are yielded correspond exactly to the result types of the parent op. It is unclear why the previous implementation worked. Clearly, the type checks have not been executed because the terminator was not `ReturnLike`, so the mismatching types passed unnoticed. I suppose that there simply is/was no other use case of using the `RegionBranchOpInterface` of the affected ops yet. Signed-off-by: Ingo Müller <ingomueller@google.com>
ingomueller-net
changed the title
yield a ReturnLike op.RegionBranchOpInterface implementations.
|
I just pushed a new commit (hopefully) fixing the failed tests. That commit increases the magnitude of the commit significantly, so I'd like another full review before proceeding. The new commit changes the behavior of the implementations of |
| @@ -104,16 +104,8 @@ transform::AlternativesOp::getEntrySuccessorOperands(RegionBranchPoint point) { | |||
|
|
|||
| void transform::AlternativesOp::getSuccessorRegions( | |||
| RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) { | |||
| for (Region &alternative : llvm::drop_begin( | |||
There was a problem hiding this comment.
Removing this does not seem right. When a region fails, the op jumps to the next region.
There was a problem hiding this comment.
In particular and unlike scf.for, transform.foreach does not pass control from one iteration of its body to the next directly; it rather passes the control back to the parent op, which then passes it back to the body for the next iteration. That can be seen by the fact that the body always gets arguments of the same type as the operands of the parent op (and none of the yielded types) and the types that are yielded correspond exactly to the result types of the parent op.
I don't follow this part. Where is the type mismatch here?
There was a problem hiding this comment.
Oh, I guess you are saying that the successor operands do not match with the block arguments of the next region.
There was a problem hiding this comment.
I think the RegionBranchOpInterface does not support the use case here. When you branch back to the parent op, there is no way to go back into the op a second time.
A "region successor" indicates the target of a branch. It can indicate
either a region of this op or this op. In the former case, the region
successor is a region pointer and a range of block arguments to which the
"successor operands" are forwarded to. In the latter case, the control flow
leaves this op and the region successor is a range of results of this op to
which the successor operands are forwarded to.
This sentence talks about results of the op, so if we would allow going back into the op, you could set the same results multiple times.
In the latter case, the control flow
leaves this op and the region successor is a range of results of this op to
which the successor operands are forwarded to.
I think we should clarify this in the documentation of RegionBranchOpInterface. (We could also allow it but most transformations on top of RegionBranchOpInterface probably assume that you cannot go back into the op.)
Do we have to implement the RegionBranchOpInterface on these transform ops at all?
There was a problem hiding this comment.
In sequence, I think the sentence matches, no? When control reaches yield, the operands of yield are forwarded to the parent op, they become the result of the op, and "control flow leaves [the] op," right?
What doesn't quite fit for foreach and alternatives is the "control flow leaves [the] op" part. One could argue that the "region successor is a [part of or a potential] range of results of [the] op," but then the control flow does not leave the op; instead it may go to the next region or re-enter the body. Plus there are rules about how to combine the different results yielded by different regions/iterations (though the types always match).
There was a problem hiding this comment.
In practice, what's the difference between "no arguments are passed" and "it isn't specified which arguments are passed"? The former does not imply that the region must have 0 arguments.
There was a problem hiding this comment.
I believe that "it isn't specified which arguments are passed" cannot be expressed currently. An empty argument list is always interpreted as "no arguments are passed."
There was a problem hiding this comment.
Yes, that's correct. But I am wondering if it makes any difference for analyses and transformations.
If we a region has an argument, but the RegionBranchOpInterface says that no arguments are forwarded to that region: what does that mean? It means that we have no idea where the data for that block argument is coming from. Maybe the op itself produces it.
Or maybe the value was actually forwarded from another region but we did not account for it in the RegionBranchOpInterface (and the terminator interface).
Does it matter which one is the case for an analysis that checks the RegionBranchOpInterface? It has to be conservative around such cases anyway.
There was a problem hiding this comment.
If we a region has an argument, but the
RegionBranchOpInterfacesays that no arguments are forwarded to that region: what does that mean?
Are you sure that that can even exist? I think I ran into failed type check that seem to check exactly that while working on the last commit...
There was a problem hiding this comment.
I think scf.forall is an example.
This PR revisits the traits and interfaces of the `alternatives`, `foreach`, and `yield`ops. First, it adds the `ReturnLike` trait to `transform.yield`. This is required in the one-shot bufferization pass since the merging of llvm#110332, which analyses any `FunctionOpInterface` and expects them to have a `ReturnLike` terminator. This uncovered problems with `alternatives` and `foreach`, which implemented the `BranchRegionOpInterface`. That interface verifies that the operands and results passed across the control flow edges from the parent op to its regions and back are equal (or compatible). Which results are passed back from regions to the parent op depend on the terminator and/or whether it is `ReturnLike`. Making `yield` a `ReturnLike` op, those checks fail without other changes. We explored and rejected the possibility to fix the implementation of `RegionBranchOpInterface` of the two concerned ops in llvm#111408. The problem is that the interface expects the result passed from a region to the parent op to *be* the result of the op, whereas in the two ops they *contribute* to the result: in `alternatives` only the operand yielded from one of the regions becomes the result whereas the others are ignored, and in `foreach` the operands from all iterations are fused into a new value that becomes the result of the op. Signed-off-by: Ingo Müller <ingomueller@google.com>
There was a problem hiding this comment.
Sorry, I missed this. This may depend on what we understand by ReturnLike, but transform.yield is likely not that. In particular, ReturnLike implies one-to-one mapping of the yielded value to the result of the parent operation or the arguments of the next region in dataflow analyses, which is not the case for transform.yield used in alternatives, or foreach.
Thanks, @ftynse, for helping us decide. I think that my earlier statement (quoted here) is then correct: one cannot assume that |
This PR adds the
ReturnLiketrait totransform.yield. This is required in the one-shot bufferization pass since the merging of #110322, which analyses anyFunctionOpInterfaceand expects them to have aReturnLiketerminator.If we give
transform.yieldtheReturnLiketrait, then the checks made byRegionBranchOpInterfacebehave differently. More concretely, the check that the operands and results passed across the control flow edges from the parent op to its regions and back are equal (or compatible). Which results are passed back from regions to the parent op depend on the terminator and/or whether it isReturnLike.This commit radically changes the implementations of the
alternatives,foreach, andsequenceops, which are the ops that useyieldas their terminator. In fact, all of these ops only ever pass control from the parent op to the region and from there back to the parent op. In particular and unlikescf.for,transform.foreachdoes not pass control from one iteration of its body to the next directly; it rather passes the control back to the parent op, which then passes it back to the body for the next iteration. That can be seen by the fact that the body always gets arguments of the same type as the operands of the parent op (and none of the yielded types) and the types that are yielded correspond exactly to the result types of the parent op.It is unclear why the previous implementation worked. Clearly, the type checks have not been executed because the terminator was not
ReturnLike, so the mismatching types passed unnoticed. I suppose that there simply is/was no other use case of using theRegionBranchOpInterfaceof the affected ops yet.