[Mlir-commits] [mlir] [mlir][transforms] Process RegionBranchOp with empty region (PR #123895)

[Xiang Li]

python3kgae wrote:

> Thanks for the PR! This seems like a pretty major bug, and I'm surprised that this is the only region affected by this bug, given how prevalent `scf.if` with an empty else region must be.
> 
> That being said, I am not convinced this is the right fix. First, if this indeed an issue, there will be other passes that suffer from the same bug. We should clarify the contract of RegionBranchOpInterface has with its operations. Should ops be allowed to have empty regions? For example, `IfOp::getRegionSuccessors` will ignore the else region if it's empty.
> 
> I personally think `scf.if` should ban empty else region and simply require it to be `else { scf.yield }` and we can sugar this in the syntax.

Thanks for the comment.
Sugar the syntax seems straightforward, we just need to update IfOp::parse.

I'm uncertain about handling the builder, specifically:
```
void IfOp::build(OpBuilder &builder, OperationState &result,
                 TypeRange resultTypes, Value cond)
```
This function doesn't create an else region. The caller will use inlineRegionBefore to add the region.
Should we create the else region implicitly? If we do, the callers might need to call eraseBlock before inlineRegionBefore in case they have an elseRegion.
On the other hand, if we don't create the else region implicitly, the caller might not add it at all.

Perhaps we should add a check to ensure the else region is not empty in IfOp::verify() and leave the builder as is?
But that will require fix all the places where the elseRegion is not add.

What do you think?

https://github.com/llvm/llvm-project/pull/123895