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

[Jeff Niu]

Mogball 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?

That builder creates the then and else regions but leaves them empty. That's fine, since that's the default behaviour of the builders. I think fixing the verifier to require a non-empty region is a good solution, and then you will have to go about and fix places in the code where the caller doesn't create an `scf.yield` in the else region. Hopefully it doesn't prove to be too much work, but this is the best way forward IMO.

But I would actually like to invite the opinion of others first, cc @matthias-springer @joker-eph 

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