Does this help mitigate your position?

@nicolasvasilache any progress on reusing the MLIR parser? I consider that refactoring as blocking for submitting this patch. I don't want us to have a custom parser copypasted here that somebody has to clean up later without a strong reason.

meta-point: @ftynse let's not review the core parser code at the top of the file, as Nicolas says that they are just copypasta from the .mlir parser and won't be in the final patch.

First round of comments.

Do you intend for this to be "approaching production quality code" and reviewed as such or still proof-of-concept level?

Presumably yield should replace the linalg and loop yields? I would add std.yield in a separate patch that refactors the other dialects to use it as well.

Update description

Address comments.

we landed a different version of this IIRC. Closing diff.

we landed a different version of this IIRC

Thanks @mehdi_amini, addressed.

update

Address comments

Update Rationale.md

LGTM from me. I think the "free returnop from funcop" discussion could go on for a while, so I would encourage you to introduce a new terminator for now so that we can land this.

Fix extraneous stuff in the diff

Address comments

Fix typo

Okay, let's land this without allowing explicit captures, given that's the most restrictive semantics. We can loosen it later if there's a compelling need.

Address comments.

LGTM after comments are addressed.

Thanks, this is a good start.

+1 This is also exactly what I wanted to say. If there were arguments in the land you were starting from (say you were inlining a call), those arguments should just get propagated and eliminated. Keeping arguments around will necessitate all kinds of tracking/bookkeeping in moving code across, reimplementing existing canonicalizations on this op and largely defeating the purpose of this op - which is to let SSA dominance and dataflow work freely from above and through it.

Update for Uday's comment.

Nice improvement! Thanks for the quick turnaround :)

All other use cases I could immediately think of actually treat 'return' the same way irrespective of whether it's inside a parent func op or an imperative one -- for eg., all region local / CFG transformations. Wouldn't that actually be an upside of reusing return? (You won't have to check for two terminators FWIW) Do you have use cases for the "something here" part below that require treating imperative and declarative parent ops of std.return differently? And I think that has to be weighed against the scenarios that require the same treatment.

Let me know if you want me to add a unittest. Couldn't find any intree users to update.

I'd like to suggest the following though exercise. Consider the following code in a pass:

Hi Sean, this patch already updates the verifier to work with both FuncOp and with execute_region! (see near Ops.cpp:1938) I see the change as trivial. :-) To generalize it to all func like ops, we need an op interface - but that doesn't complicate the verifier in any way.

FWIW, I also think that we should not use std.return for inlined_call/execute_region. I can’t think of a nontrivial verifier that we could write that would allow both that use and the more typical use in FuncOp.

Since it seems the discussion is a bit stalled, maybe I can make a suggestion.

I couldn't think of a great way to test this. The one place I can think of that could potentially benefit from this uses updatePredTerms=false because it erases successor operands manually for other reasons:
https://github.com/llvm/llvm-project/blob/e8358455a2b662bec59bc3971c18346800d7cb00/mlir/lib/Transforms/Utils/RegionUtils.cpp#L278

Fix review feeback.

Fix review feedback.

Thanks. This makes sense now that we have include guards.