Extend/generalize the 'return' op (ReturnOp)

76 views
Skip to first unread message

Uday Bondhugula

unread,
Dec 19, 2019, 8:38:32 AM12/19/19
to MLIR
Hi all,

I felt the changes in this PR may require discussion. I'm thus posting it here. 

This generalization is also needed to support the proposed affine.graybox in its full power: 
which could be the first use case exercising this additional freedom --- but there are clearly many more.

Best,
Uday



Alex Zinenko

unread,
Dec 19, 2019, 10:23:53 AM12/19/19
to Uday Bondhugula, MLIR
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

func @foo() {
  affine.if ... {
    std.return // this returns from the function
  }
}

func @bar() {
  affine.greybox {
    std.return // this return from the region
  }
}

func @baz {
  affine.greybox {
    affine.if {
      std.return // 'if' says it should return from the function, but 'greybox' says return from region...
    }
  }
}

Adding a specific affine.yield does not cost that much...

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.
To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/df062b81-2443-414a-adbd-34d9c79c7261%40tensorflow.org.


Uday Bondhugula

unread,
Dec 19, 2019, 10:37:13 AM12/19/19
to MLIR


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it.  This also happens to be consistent with the documentation on a region's control flow in LangRef (reproduced below). It was actually the implementation that was incomplete!

Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only flow into the first block of the region, but can flow out of the region at the end of any of the contained blocks (This behavior is similar to that of a function body in most programming languages). When exiting a Region, control is returned to the enclosing operation.

~ Uday

 

func @foo() {
  affine.if ... {
    std.return // this returns from the function
  }
}

func @bar() {
  affine.greybox {
    std.return // this return from the region
  }
}

func @baz {
  affine.greybox {
    affine.if {
      std.return // 'if' says it should return from the function, but 'greybox' says return from region...
    }
  }
}

Adding a specific affine.yield does not cost that much...

On Thu, Dec 19, 2019 at 2:38 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:
Hi all,

I felt the changes in this PR may require discussion. I'm thus posting it here. 

This generalization is also needed to support the proposed affine.graybox in its full power: 
which could be the first use case exercising this additional freedom --- but there are clearly many more.

Best,
Uday



--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ml...@tensorflow.org.

Alex Zinenko

unread,
Dec 19, 2019, 11:17:11 AM12/19/19
to Uday Bondhugula, MLIR


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.


This also happens to be consistent with the documentation on a region's control flow in LangRef (reproduced below). It was actually the implementation that was incomplete!

Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only flow into the first block of the region, but can flow out of the region at the end of any of the contained blocks (This behavior is similar to that of a function body in most programming languages). When exiting a Region, control is returned to the enclosing operation.


Indeed, it's a good point. I should rephrase this more carefully. And it's reasonable to think about locality of flow across nested regions, and even more about "declaration-only" regions such as module bodies.

It should probably say "In a region block, a terminator may transfer the flow of control to any other non-entry block of the same region, or to any enclosing operation". This would also support use cases like "break from multiple loops with one instruction".


To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.
To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/3820c08a-1d08-47e1-aed3-21b744c92f7a%40tensorflow.org.

Uday Bondhugula

unread,
Dec 19, 2019, 12:36:07 PM12/19/19
to MLIR


On Thursday, December 19, 2019 at 9:47:11 PM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.

I still don't see your point here. std.return doesn't return to the nearest function - it transfers control to its parent op just like the current doc says. In the current codebase, one can use std.return only from the top level of a func op. My patch changes that and allows one to use it from the top level of other ops that have a region (if those ops allow it). (For eg. you can use a return from the top level of an affine.graybox.) You still won't be able to use it from a point that is not the top level of that op.  Could you look at the update to the 'return' op documentation I made in the PR?

 


This also happens to be consistent with the documentation on a region's control flow in LangRef (reproduced below). It was actually the implementation that was incomplete!

Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only flow into the first block of the region, but can flow out of the region at the end of any of the contained blocks (This behavior is similar to that of a function body in most programming languages). When exiting a Region, control is returned to the enclosing operation.


Indeed, it's a good point. I should rephrase this more carefully. And it's reasonable to think about locality of flow across nested regions, and even more

No, there isn't a need to update it!

~ Uday


 

Geoffrey Martin-Noble

unread,
Dec 19, 2019, 2:25:19 PM12/19/19
to Uday Bondhugula, MLIR
Here's the previous discussion on this.

To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.
To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/a5a54a45-60cb-4d79-af96-acec24bc145f%40tensorflow.org.

Alex Zinenko

unread,
Dec 19, 2019, 2:25:27 PM12/19/19
to Uday Bondhugula, MLIR
On Thu, Dec 19, 2019 at 6:36 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 9:47:11 PM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.

I still don't see your point here. std.return doesn't return to the nearest function - it transfers control to its parent op just like the current doc says. In the current codebase, one can use std.return only from the top level of a func op. My patch changes that and allows one to use it from the top level of other ops that have a region (if those ops allow it). (For eg. you can use a return from the top level of an affine.graybox.) You still won't be able to use it from a point that is not the top level of that op.  Could you look at the update to the 'return' op documentation I made in the PR?

We get into non-trivial interaction between the semantics of _all_ ops enclosing "return" to understand what it actually does, instead of disallowing it completely (now) or having it return from the nearest function. The current doc for "std.return" says "The return terminator operation represents the completion of a function, and produces the result values.", there's nothing about enclosing operations. The doc on region mentions "exiting a Region", but it is actually left unmodified from some earlier version of the region proposal that was having different assumptions about regions, in particular regions being able to omit terminators. We went for region _pretty printing_ being able to omit terminators instead, so it is unclear what "exiting a region" actually means now :) It used to mean "reaching the end of the operation list without seeing a terminator" (I wrote the proposal).


  


This also happens to be consistent with the documentation on a region's control flow in LangRef (reproduced below). It was actually the implementation that was incomplete!

Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only flow into the first block of the region, but can flow out of the region at the end of any of the contained blocks (This behavior is similar to that of a function body in most programming languages). When exiting a Region, control is returned to the enclosing operation.


Indeed, it's a good point. I should rephrase this more carefully. And it's reasonable to think about locality of flow across nested regions, and even more

No, there isn't a need to update it!

I still want support for something like if-guarded "break" in a loop. Unless we figure out some forwarding that the "if" would do to forward the fact that it was terminated by "break" to its parent, which may in turn need to forward it further up, I don't see a way around this.
 
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.
To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/a5a54a45-60cb-4d79-af96-acec24bc145f%40tensorflow.org.


--
-- Alex

Uday Bondhugula

unread,
Dec 19, 2019, 8:33:13 PM12/19/19
to MLIR


On Friday, December 20, 2019 at 12:55:19 AM UTC+5:30, Geoffrey Martin-Noble wrote:
Here's the previous discussion on this.

This PR (#330) does not add or allow returns from the region of affine.for or affine.if; those still use the affine.terminator. It's the affine.graybox that would allow a return from its top level. affine.for/if maintain the same control flow property, and affine passes remain unaffected. That previous discussion appears to have mixed up / over-generalized the concept by thinking of return as being associated with *some* enclosing affine.for/if, which breaks the structured control flow restrictions of the latter. With the change in this PR, the return would still always transfer control to its parent op (immediately enclosing op), and you still can't use a return from within the top-level of an affine.for or affine.if.

~ Uday
 

Uday Bondhugula

unread,
Dec 19, 2019, 8:53:21 PM12/19/19
to MLIR


On Friday, December 20, 2019 at 12:55:27 AM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019 at 6:36 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 9:47:11 PM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.

I still don't see your point here. std.return doesn't return to the nearest function - it transfers control to its parent op just like the current doc says. In the current codebase, one can use std.return only from the top level of a func op. My patch changes that and allows one to use it from the top level of other ops that have a region (if those ops allow it). (For eg. you can use a return from the top level of an affine.graybox.) You still won't be able to use it from a point that is not the top level of that op.  Could you look at the update to the 'return' op documentation I made in the PR?

We get into non-trivial interaction between the semantics of _all_ ops enclosing "return" to understand what it actually does, instead of disallowing it completely (now) or having it return from the nearest function. The current doc for "std.return" says "The return terminator operation represents the completion of a function, and produces the result values.", there's nothing about enclosing operations. The doc on region mentions "exiting a Region", but it is actually left unmodified from some earlier version of the region proposal that was having different assumptions about regions, in particular regions being able to omit terminators. We went for region _pretty printing_ being able to omit terminators instead, so it is unclear what "exiting a region" actually means now :) It used to mean "reaching the end of the operation list without seeing a terminator" (I wrote the proposal).


  


This also happens to be consistent with the documentation on a region's control flow in LangRef (reproduced below). It was actually the implementation that was incomplete!

Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only flow into the first block of the region, but can flow out of the region at the end of any of the contained blocks (This behavior is similar to that of a function body in most programming languages). When exiting a Region, control is returned to the enclosing operation.


Indeed, it's a good point. I should rephrase this more carefully. And it's reasonable to think about locality of flow across nested regions, and even more

No, there isn't a need to update it!

I still want support for something like if-guarded "break" in a loop. Unless we figure out some forwarding that the "if" would do to forward the fact that it was terminated by "break" to its parent, which may in turn need to forward it further up, I don't see a way around this.

But that's what the affine.graybox allows you to do. C style if's with continue/breaks are represented. Please see 'Example 1':
(A C-style 'for' with a 'break' under an 'if' would be a list of blocks in a graybox as opposed to any affine.for or affine.if. From the top, an affine pass would treat the graybox as an atomic op, and then treat each of the blocks in the graybox as a separate unit -- runOn<each of those>. Strangely, this example would be something representable in a high level form with affine ops, but not purely with loop dialects ops. The lower for such a 2-d loop nest with an if/break inside would first lower the outer affine.for into a loop.for which would then become std control flow (list of blocks), and then affine.graybox held inside gets lowered by the same logic that the inliner uses. Although this is a separate discussion for the future, if you'd like to preserve two step affine -> loop, loop -> std in the presence of an affine.graybox, you'll need a loop.graybox or an equivalent concept. )

~ Uday

 
 

Compiler Developer

unread,
Dec 19, 2019, 11:04:23 PM12/19/19
to MLIR
Hi,

Why not create the operations like break, continue?  This is needed for loop.for as well. 

What happens when there is an actual function return inside the loop? 

Thanks!

Uday Bondhugula

unread,
Dec 19, 2019, 11:32:30 PM12/19/19
to MLIR


On Friday, December 20, 2019 at 9:34:23 AM UTC+5:30, Compiler Developer wrote:
Hi,

What happens when there is an actual function return inside the loop? 

There can't be. You'll get a verifier failure (irrespective of this PR).
 
~ Uday

Uday Bondhugula

unread,
Dec 20, 2019, 12:10:21 AM12/20/19
to MLIR


On Friday, December 20, 2019 at 12:55:27 AM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019 at 6:36 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 9:47:11 PM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.

I still don't see your point here. std.return doesn't return to the nearest function - it transfers control to its parent op just like the current doc says. In the current codebase, one can use std.return only from the top level of a func op. My patch changes that and allows one to use it from the top level of other ops that have a region (if those ops allow it). (For eg. you can use a return from the top level of an affine.graybox.) You still won't be able to use it from a point that is not the top level of that op.  Could you look at the update to the 'return' op documentation I made in the PR?

We get into non-trivial interaction between the semantics of _all_ ops enclosing "return" to understand what it actually does, instead of disallowing it completely (now) or having it return from the nearest function. The current doc for "std.return" says "The return terminator operation represents the

I have a feeling that you may be missing the 'top-level' part when you say 'enclose' (perhaps due to the wording of the commit message). The std.return op can only be used from the top-level of a function op (the way it is now) or the top level of another op with a region that allows it (after this PR) - affine.graybox will be an example. It isn't possible to (whether before or after this PR) to use it from within an affine.for or an affine.if; these blocks still use "affine.terminator" for their terminators. If you have a completely unknown op ("unknown"() ({...}) : () -> ()) that uses a return in its region, its semantics would be unknown and you'd anyway not convert/lower to std.  FuncOp is currently the only registered op that allows a return (*only at its top level*). affine.graybox will be another op that will allow a return at its top level. In both cases, the semantics of the ensuing transfer of control are well defined. Why and where would you need to understand the semantics of an unknown op having a return in its holding region block? For the known ops, the op's implementation and lowering will define what the return means and how it should be lowered.

~ Uday

River Riddle

unread,
Dec 20, 2019, 12:45:16 AM12/20/19
to Uday Bondhugula, MLIR
I don't understand what you mean here. Are you implying that the lowering is based on the parent op? i.e. a ReturnOp in an affine.graybox would be lowered differently than a ReturnOp in a FuncOp? Unless you do some pre-analysis it isn't really possible to detect this during lowering, i.e. if the FuncOp/AffineGreyBox is being lowered into something else this will happen before the ReturnOp is even visited. Unless you are suggesting that the parent operation must always lower the return, which isn't viable.

-- River
 

~ Uday

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.

Mehdi AMINI

unread,
Dec 20, 2019, 1:22:52 AM12/20/19
to Uday Bondhugula, MLIR
Hi,

I'm missing the fundamental motivation? You could just as well have another op to return from the graybox? 

In the past I advocated for generalizing std.return, but:
1) creating another op is "trivial".
2) the strong verifier in std.return can catch transformation bugs where we'd transform the IR in an unwanted state.

-- 
Mehdi

Compiler Developer

unread,
Dec 20, 2019, 1:25:20 AM12/20/19
to MLIR
Yes, but isn't that what graybox needs to capture?

 " The op allows the polyhedral form to be used without the need for outlining to functions, and without the need to turn affine ops such as affine.for, affine.if into standard unrestricted for's and if's respectively." - https://github.com/bondhugula/mlir/blob/graybox/rfc/rfc-graybox.md#example-1

What happens when there are loops like below (Example code 1)  where we know it is going to be affine? There would also be cases where some transformations (peeling, fission, switch, unswitch, invariant code motion etc) on the unstrucutred loops which yeilds affine form. Especially for the languages like C, Fortran, no one would want to write such redundant transformations at AST level to be able to emit affine.for.  It would not be good to move them to CFG based loops and then convert it to higher form.

Ideal solution would be to model them in higher level loop forms with operations defined for break/ continue/ return, etc. and then lower to any of afine.for, loop.for,gpu.func(), openmp.for, CFG based loop, etc based on whichever is suitable.

Example code 1:

int some_func() {

 <outer affine loops>

 for (int i = 0; i < 10; i++) {
   if (i  < 0) {  
   retrurn <some_val> // This will be DCE'd 
 }

 <inner affine loops>
}
} 
 
~ Uday

Uday Bondhugula

unread,
Dec 20, 2019, 1:29:11 AM12/20/19
to MLIR


On Friday, December 20, 2019 at 11:15:16 AM UTC+5:30, River Riddle wrote:


On Thu, Dec 19, 2019 at 9:10 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Friday, December 20, 2019 at 12:55:27 AM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019 at 6:36 PM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 9:47:11 PM UTC+5:30, Alex Zinenko wrote:


On Thu, Dec 19, 2019, 16:37 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Thursday, December 19, 2019 at 8:53:53 PM UTC+5:30, Alex Zinenko wrote:
IIRC, this has been discussed several times in different contexts. The common argument against this is that, without knowing the semantics of a specific region, it's unclear where should "std.return" transfer the control flow.

But that's left to the semantics of the op, and the op's implementation to deal with it. 

Having the semantics of an op depend on the surrounding op, especially from a different dialect, sounds too brittle to me. And you would essentially disallow the early-return pattern in functions.

Note that I'm not opposed to having a terminator that unconditionally transfers the control flow back to the surrounding op. It just shouldn't be "return", which returns from the nearest function, and being used otherwise is likely to be a perpetual source of confusion.

I still don't see your point here. std.return doesn't return to the nearest function - it transfers control to its parent op just like the current doc says. In the current codebase, one can use std.return only from the top level of a func op. My patch changes that and allows one to use it from the top level of other ops that have a region (if those ops allow it). (For eg. you can use a return from the top level of an affine.graybox.) You still won't be able to use it from a point that is not the top level of that op.  Could you look at the update to the 'return' op documentation I made in the PR?

We get into non-trivial interaction between the semantics of _all_ ops enclosing "return" to understand what it actually does, instead of disallowing it completely (now) or having it return from the nearest function. The current doc for "std.return" says "The return terminator operation represents the

I have a feeling that you may be missing the 'top-level' part when you say 'enclose' (perhaps due to the wording of the commit message). The std.return op can only be used from the top-level of a function op (the way it is now) or the top level of another op with a region that allows it (after this PR) - affine.graybox will be an example. It isn't possible to (whether before or after this PR) to use it from within an affine.for or an affine.if; these blocks still use "affine.terminator" for their terminators. If you have a completely unknown op ("unknown"() ({...}) : () -> ()) that uses a return in its region, its semantics would be unknown and you'd anyway not convert/lower to std.  FuncOp is currently the only registered op that allows a return (*only at its top level*). affine.graybox will be another op that will allow a return at its top level. In both cases, the semantics of the ensuing transfer of control are well defined. Why and where would you need to understand the semantics of an unknown op having a return in its holding region block? For the known ops, the op's implementation and lowering will define what the return means and how it should be lowered.
I don't understand what you mean here. Are you implying that the lowering is based on the parent op? i.e. a ReturnOp in an affine.graybox would be lowered differently than a ReturnOp in a FuncOp? Unless you do some pre-analysis it isn't really possible to detect this during lowering, i.e. if the FuncOp/AffineGreyBox is

There is absolutely no difference between "lowering an affine.graybox op" and inlining a function CFG (list of basic blocks) into the top level of another function (i.e., the call site is at the top level). The affine graybox would just disappear post inlining. Are you assuming that the lowering would solely be a composition of dialect conversion rewrites applied through a greedy rewriter? I haven't thought of the exact implementation details of an affine.graybox lowering, but I don't see any conceptual issue.

~ Uday

 
being lowered into something else this will happen before the ReturnOp is even visited. Unless you are suggesting that the parent operation must always lower the return, which isn't viable.

-- River
 

~ Uday

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ml...@tensorflow.org.

Uday Bondhugula

unread,
Dec 20, 2019, 2:49:16 AM12/20/19
to MLIR
Mehdi,

There isn't a need for a new op - because I don't see an issue with reusing the std.return op if control is to be transferred to parent op. Reg. the return op verifier, there isn't a downside either. Can you see the update to verify(ReturnOp) on the PR? A missing return on a FuncOp will still be caught, and there isn't a win on disallowing return at the region top levels of other ops that want to. Creating a new return op wherever control is to be transferred to the parent op would be pure duplication - like for affine.graybox. If the name is confusing (I don't really see that to be an issue as well), STD.return can be renamed std.return_to_parent. But this is also unnecessary. I do understand that someone new might initially think of STD.return as a C function return.

Uday

Uday Bondhugula

unread,
Dec 20, 2019, 2:50:07 AM12/20/19
to MLIR

Uday Bondhugula

unread,
Dec 20, 2019, 3:47:59 AM12/20/19
to MLIR


On Friday, December 20, 2019 at 11:55:20 AM UTC+5:30, Compiler Developer wrote:

On Friday, December 20, 2019 at 10:02:30 AM UTC+5:30, Uday Bondhugula wrote:


On Friday, December 20, 2019 at 9:34:23 AM UTC+5:30, Compiler Developer wrote:
Hi,

What happens when there is an actual function return inside the loop? 

There can't be. You'll get a verifier failure (irrespective of this PR).

Yes, but isn't that what graybox needs to capture?

The return, when it exists, will be from the top level of the graybox - you never need it inside the block of an affine.for. The terminator for an affine.for/if is still "affine.terminator".  Please see for example:

~ Uday

Alex Zinenko

unread,
Dec 20, 2019, 4:27:14 AM12/20/19
to Uday Bondhugula, MLIR
I take your point about unknown ops.

I share River's concern below about lowering (although I'd argue we shouldn't define the semantics of ops in terms of lowering). It seems plausible that the "return" you would use in "affine.greybox" will be lowered away before you hit the potentially problematic case of unconditionally lowering "std.return" to "llvm.return" or its equivalent in other targets.

I don't agree with the cost-benefit analysis here. Your proposal spares the introduction of a trivial terminator operation in the Affine dialect. It introduces a dependence between Standard dialect and Affine dialect in terms of ops. It creates a lot of confusion as the length of this discussion demonstrates. There are no immediate other users of this extension than "affine.greybox". I would take the cost of introducing a new operation now, and eventually generalizing it to "std.return_to_parent" when the need arises.

To avoid conflating the discussion, I'll factor out the LangRef change into a separate thread.
 

~ Uday

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.

Compiler Developer

unread,
Dec 20, 2019, 4:41:09 AM12/20/19
to MLIR

1.  

int func(..) {
<outer affine loop> {

 for (int i = 0; i < 10; i++) {
   if (i  < 0) {   
   retrurn <some_val>  // This is dead code.
  <affine operations>
 }
}
...
}

Above code will have two std.return operations inside the affine.graybox? One for C function and another for affine.graybox region?

2. About the affinegraybox op:  I am still confused as to why not have break/continue instead of CFG? In the above case, there would be transformations to recover affine.for from the CFG based grayboxop? 

Uday Bondhugula

unread,
Dec 20, 2019, 5:48:00 AM12/20/19
to MLIR


On Friday, December 20, 2019 at 3:11:09 PM UTC+5:30, Compiler Developer wrote:

1.  

int func(..) {
<outer affine loop> {

 for (int i = 0; i < 10; i++) {
   if (i  < 0) {   
   retrurn <some_val>  // This is dead code.
  <affine operations>
 }
}
...
}

Above code will have two std.return operations inside the affine.graybox? One for C function and another for affine.graybox region?

Please either write entirely in C or MLIR - I can't really tell what your example from the mix above is!
 

2. About the affinegraybox op:  I am still confused as to why not have break/continue instead of CFG? In the above case, there would be transformations to recover affine.for from the CFG based grayboxop? 

Can you please post questions on affine.graybox on a separate thread? This one isn't really about its design choices.

~ Uday

Alex Zinenko

unread,
Dec 20, 2019, 5:53:08 AM12/20/19
to Uday Bondhugula, MLIR
On Fri, Dec 20, 2019 at 11:48 AM 'Uday Bondhugula' via MLIR <ml...@tensorflow.org> wrote:


On Friday, December 20, 2019 at 3:11:09 PM UTC+5:30, Compiler Developer wrote:

1.  

int func(..) {
<outer affine loop> {

 for (int i = 0; i < 10; i++) {
   if (i  < 0) {   
   retrurn <some_val>  // This is dead code.
  <affine operations>
 }
}
...
}

Above code will have two std.return operations inside the affine.graybox? One for C function and another for affine.graybox region?

Please either write entirely in C or MLIR - I can't really tell what your example from the mix above is!

I'd say that before DCE the code is not affine in the first place. Early returns are not allowed in (conventional) SCoPs and the code should be transformed first.

That being said, it may make sense to have "break" and "continue" in the "loops" dialect eventually. I will be happy to review a proposal in that direction outside of this conversation.
 
 

2. About the affinegraybox op:  I am still confused as to why not have break/continue instead of CFG? In the above case, there would be transformations to recover affine.for from the CFG based grayboxop? 

Can you please post questions on affine.graybox on a separate thread? This one isn't really about its design choices.

+1.
 

~ Uday
 



On Friday, December 20, 2019 at 2:17:59 PM UTC+5:30, Uday Bondhugula wrote:


On Friday, December 20, 2019 at 11:55:20 AM UTC+5:30, Compiler Developer wrote:

On Friday, December 20, 2019 at 10:02:30 AM UTC+5:30, Uday Bondhugula wrote:


On Friday, December 20, 2019 at 9:34:23 AM UTC+5:30, Compiler Developer wrote:
Hi,

What happens when there is an actual function return inside the loop? 

There can't be. You'll get a verifier failure (irrespective of this PR).

Yes, but isn't that what graybox needs to capture?

The return, when it exists, will be from the top level of the graybox - you never need it inside the block of an affine.for. The terminator for an affine.for/if is still "affine.terminator".  Please see for example:

~ Uday
 

 " The op allows the polyhedral form to be used without the need for outlining to functions, and without the need to turn affine ops such as affine.for, affine.if into standard unrestricted for's and if's respectively." - https://github.com/bondhugula/mlir/blob/graybox/rfc/rfc-graybox.md#example-1

What happens when there are loops like below (Example code 1)  where we know it is going to be affine? There would also be cases where some transformations (peeling, fission, switch, unswitch, invariant code motion etc) on the unstrucutred loops which yeilds affine form. Especially for the languages like C, Fortran, no one would want to write such redundant transformations at AST level to be able to emit affine.for.  It would not be good to move them to CFG based loops and then convert it to higher form.

Ideal solution would be to model them in higher level loop forms with operations defined for break/ continue/ return, etc. and then lower to any of afine.for, loop.for,gpu.func(), openmp.for, CFG based loop, etc based on whichever is suitable.

Example code 1:

int some_func() {

 <outer affine loops>

 for (int i = 0; i < 10; i++) {
   if (i  < 0) {  
   retrurn <some_val> // This will be DCE'd 
 }

 <inner affine loops>
}
} 
 
~ Uday

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.

Compiler Developer

unread,
Dec 20, 2019, 6:15:42 AM12/20/19
to MLIR
Here is the a loop nest example:

int func(int A[10][10]) {
  int some_cond = -10;
  for (int i = 0 ; i < 10; ++i) {
    for (int j = 0; j < 10; ++j) {
      if (j < some_cond) {
        return 0;
      }
      A[i][j] = 10;
    }
  }
  return 1;
}

My qeustion is, what happens to the loop nest?  outer loop should be affine.for and inner one will be affine.graybox op? OR affine dialect cannot be used? If the affine.graybox can be used, will there be two std.return statements?

>>>> I'd say that before DCE the code is not affine in the first place. Early returns are not allowed in (conventional) SCoPs and the code should be transformed first.

So I have to convert it back from loop.for/ CFG based loops to affine.for back?
To unsubscribe from this group and stop receiving emails from it, send an email to ml...@tensorflow.org.


--
-- Alex

Uday Bondhugula

unread,
Jan 6, 2020, 5:00:23 AM1/6/20
to MLIR
The return op needed for affine.graybox has _*exactly*_ the same semantics as std.return -- creating a new op for the affine dialect would lead to a perfect clone -- ditto 1:1 code duplication everywhere. If the name is the confusion, that should be addressed by renaming "return" to std.return_to_parent (even this I feel, as I mentioned isn't needed so long as the it's clarified in the doc which the patch I submitted also does) instead of creating a duplicate op that mirrors that concept.

Coming back to this patch ( https://reviews.llvm.org/D71961 ), which is about allowing the use of return wherever the *same* concept is needed (returning to immediate parent op), the patch is only fixing the confusion surrounding the existing return op -- by clarifying it in the doc and allowing wider use consistent with its *existing* semantics.

~ Uday

> To avoid conflating the discussion, I'll factor out the LangRef change into a separate thread.
 

~ Uday

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ml...@tensorflow.org.


--
-- Alex
Reply all
Reply to author
Forward
0 new messages