- User Since
- Aug 20 2012, 11:51 AM (398 w, 1 d)
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.
Fri, Apr 3
First round of comments.
Mon, Mar 30
Do you intend for this to be "approaching production quality code" and reviewed as such or still proof-of-concept level?
Sat, Mar 28
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.
Fri, Mar 27
Thu, Mar 26
Wed, Mar 25
we landed a different version of this IIRC. Closing diff.
we landed a different version of this IIRC
Tue, Mar 24
Thanks @mehdi_amini, addressed.
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
Thu, Mar 19
Wed, Mar 18
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.
Tue, Mar 17
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.
Fri, Mar 13
Wed, Mar 11
Update for Uday's comment.
Nice improvement! Thanks for the quick turnaround :)
Mon, Mar 9
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.
Mar 5 2020
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.
Feb 19 2020
Feb 14 2020
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:
Feb 13 2020
Jan 29 2020
Fix review feeback.
Fix review feedback.
Dec 23 2019
Nov 19 2019
Thanks. This makes sense now that we have include guards.