Hi Eli:>> semantics of the fork intrinsic... thinking about it a bit more, I think >> you're going to run into problems with trying to keep around a return block >> through optimizations:How about this? Make all control flow explicit (duh). declare i8 coro.suspend([...]) returns: 0 - resume 1 - cleanup anything else - suspend Now we can get rid of the coro.fork altogether. [...] %0 = call i8 @llvm.coro.suspend([...]) switch i8 %0, label %suspend [ i32 0, label %resume, i32 1, label %cleanup] suspend: call void @llvm.coro.end() br %return.block We no longer have to do any special handling of return block as coro.end takes care of it. Lowering of coro.suspend X 1. Split block at coro.suspend, new block becomes the resume label X 2. RAUW coro.suspend with 0 in f.resume and 1 in f.destroy 3. Replace coro.suspend with 'br %suspend' coro.end expands as before: in f => no-op in f.resume/f.destroy => ret void (+ fancy things in landing pads) Gor On Thu, Jun 9, 2016 at 4:50 PM, Eli Friedman <eli.friedman at gmail.com> wrote:> On Thu, Jun 9, 2016 at 2:33 PM, Gor Nishanov <gornishanov at gmail.com> wrote: >> >> Lowering of coro.suspend number X: >> >> 1) split block at coro.suspend, new block becomes jump point for >> resumption X >> 2) RAUW coro.suspend with i1 true in resume clone i1 false in destroy >> clone >> 3) replace coro.suspend with br %return.block in 'f', 'ret void' in clones >> >> Scratch the proposed corosuspend instruction. I think the intrinsic will >> work >> just fine! > > > That sounds right. > > > If you're going down that route, that still leaves the question of the > semantics of the fork intrinsic... thinking about it a bit more, I think > you're going to run into problems with trying to keep around a return block > through optimizations: > > [...] > %first.return = call i1 @llvm.experimental.coro.fork() > %cmp = icmp eq i32 %x, 0 > br i1 %cmp, label %block1, label %block2 > > block1: > [...] > br i1 %first.return, label %coro.return, label %coro.start > > block2: > [...] > br i1 %first.return, label %coro.return, label %coro.start > > coro.return: > %xxx = phi i32 [ %a, %block1 ], [ %b, %block2 ] > call void @f(i32 %xxx) > ret i8* %frame > > (Now you can't trivially insert branches to the return block because of the > PHI node.) > > -Eli
>> If you're going down that route, that still leaves the question of the >> semantics of the fork intrinsic... thinking about it a bit more, I think >> you're going to run into problems with trying to keep around a return block >> through optimizations:Thinking about it a bit more, it is even simpler, I don't have to involve coro.end in this case at all and should not worry about branches, RAUW-ing coro.suspend with an appropriate constant takes care of it all. Here is an updated version. 1) coro.fork is gone, gone gone!!!! 2) coro.suspend is changed as follows: declare i8 coro.suspend([...]) returns: 0 - in resume clone 1 - in destroy clone -1 - in the original function Usage: [...] %0 = call i8 @llvm.coro.suspend([...]) switch i8 %0, label %return [ i32 0, label %resume, i32 1, label %cleanup] Lowering of coro.suspend X In the original function: * RAUW coro.suspend to -1 * Remove coro.suspend In both clones: * RAUW coro.suspend to 0 in `f.resume` and 1 in `f.destroy` * Split block after coro.suspend, new block becomes the resume label X * replace coro.suspend with - `ret void` in resume clone - @llvm.trap() in destroy clone (in destroy clone none of the suspends points should be reachable, if they are it is a front end bug) Much prettier than before :-) Gor On Thu, Jun 9, 2016 at 5:31 PM, Gor Nishanov <gornishanov at gmail.com> wrote:> Hi Eli: > >>> semantics of the fork intrinsic... thinking about it a bit more, I think >>> you're going to run into problems with trying to keep around a return block >>> through optimizations: > > How about this? Make all control flow explicit (duh). > > declare i8 coro.suspend([...]) > > returns: > 0 - resume > 1 - cleanup > anything else - suspend > > Now we can get rid of the coro.fork altogether. > > [...] > %0 = call i8 @llvm.coro.suspend([...]) > switch i8 %0, label %suspend [ i32 0, label %resume, > i32 1, label %cleanup] > suspend: > call void @llvm.coro.end() > br %return.block > > We no longer have to do any special handling of return block as coro.end takes > care of it. > > Lowering of coro.suspend X > > 1. Split block at coro.suspend, new block becomes the resume label X > 2. RAUW coro.suspend with 0 in f.resume and 1 in f.destroy > 3. Replace coro.suspend with 'br %suspend' > > coro.end expands as before: > > in f => no-op > in f.resume/f.destroy => ret void (+ fancy things in landing pads) > > Gor > > On Thu, Jun 9, 2016 at 4:50 PM, Eli Friedman <eli.friedman at gmail.com> wrote: >> On Thu, Jun 9, 2016 at 2:33 PM, Gor Nishanov <gornishanov at gmail.com> wrote: >>> >>> Lowering of coro.suspend number X: >>> >>> 1) split block at coro.suspend, new block becomes jump point for >>> resumption X >>> 2) RAUW coro.suspend with i1 true in resume clone i1 false in destroy >>> clone >>> 3) replace coro.suspend with br %return.block in 'f', 'ret void' in clones >>> >>> Scratch the proposed corosuspend instruction. I think the intrinsic will >>> work >>> just fine! >> >> >> That sounds right. >> >> >> If you're going down that route, that still leaves the question of the >> semantics of the fork intrinsic... thinking about it a bit more, I think >> you're going to run into problems with trying to keep around a return block >> through optimizations: >> >> [...] >> %first.return = call i1 @llvm.experimental.coro.fork() >> %cmp = icmp eq i32 %x, 0 >> br i1 %cmp, label %block1, label %block2 >> >> block1: >> [...] >> br i1 %first.return, label %coro.return, label %coro.start >> >> block2: >> [...] >> br i1 %first.return, label %coro.return, label %coro.start >> >> coro.return: >> %xxx = phi i32 [ %a, %block1 ], [ %b, %block2 ] >> call void @f(i32 %xxx) >> ret i8* %frame >> >> (Now you can't trivially insert branches to the return block because of the >> PHI node.) >> >> -Eli
On Fri, Jun 10, 2016 at 6:36 AM, Gor Nishanov <gornishanov at gmail.com> wrote:> >> If you're going down that route, that still leaves the question of the > >> semantics of the fork intrinsic... thinking about it a bit more, I think > >> you're going to run into problems with trying to keep around a return > block > >> through optimizations: > > Thinking about it a bit more, it is even simpler, I don't have to involve > coro.end in this case at all and should not worry about branches, RAUW-ing > coro.suspend with an appropriate constant takes care of it all. > > Here is an updated version. > 1) coro.fork is gone, gone gone!!!! > 2) coro.suspend is changed as follows: > > declare i8 coro.suspend([...]) > > returns: > 0 - in resume clone > 1 - in destroy clone > -1 - in the original function > > Usage: > > [...] > %0 = call i8 @llvm.coro.suspend([...]) > switch i8 %0, label %return [ i32 0, label %resume, > i32 1, label %cleanup] > > Lowering of coro.suspend X > > In the original function: > * RAUW coro.suspend to -1 > * Remove coro.suspend > > In both clones: > * RAUW coro.suspend to 0 in `f.resume` and 1 in `f.destroy` > * Split block after coro.suspend, new block becomes the resume label X > * replace coro.suspend with > - `ret void` in resume clone > - @llvm.trap() in destroy clone > (in destroy clone none of the suspends points should be reachable, > if they are it is a front end bug) > > Much prettier than before :-) > > >I'm not sure this quite works the way you want it to in terms of the optimizer. For example: block1: suspend switch (resume, destroy, return) resume: store zero to global @g [...] destroy: store zero to global @g [...] return: store zero to global @g [...] Naively, you would expect that it would be legal to hoist the store... but that breaks your coroutine semantics because the global could be mutated between the first return and the resume. Probably the most natural way to solve this is the two-function approach I suggested earlier... that way, the first call to suspend isn't special, so everything just works. There might be some other way to solve it, but I think it just becomes more awkward. For example, you could add a boolean that tracks whether the function has been suspended yet, and explicitly reroute the control flow to conditionally perform one-time operations before each call to suspend. But then you need a giant switch statement or a separate one-time cleanup function to actually route the control flow correctly. -Eli -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160610/4344a44c/attachment.html>