ychen added a comment. In D97915#2859237 <https://reviews.llvm.org/D97915#2859237>, @ChuanqiXu wrote:
> In D97915#2848816 <https://reviews.llvm.org/D97915#2848816>, @ychen wrote: > >>> Thanks for clarifying. Let's solve the semantics problem first. >>> With the introduction about 'raw frame', I think it's necessary to >>> introduce this concept in the section 'Switched-Resume Lowering' or even >>> the section 'Introduction' in the document. Add a section to tell the >>> terminology is satisfied too. >> >> Done. >> >>> Then why we defined both 'llvm.coro.raw.frame.ptr.offset' and >>> 'llvm.coro.raw.frame.ptr.addr' together? It looks like refer to the same >>> value finally. It looks like 'llvm.coro.raw.frame.ptr.offset' are trying to >>> solve the problem about memory leak. But I think we could use >>> llvm.coro.raw.frame.ptr.addr directly instead of traversing the frame >>> (Maybe we need to add an intrinsic `llvm.coro.raw.size`). Then we can omit >>> a field in the frame to save space. >> >> ("llvm.coro.raw.frame.ptr.offset" is an offset from coroutine frame address >> instead of raw frame pointer) >> >> Apologies for the confusion. I've briefly explained it here >> https://reviews.llvm.org/D102145#2752445 I think it is not clear. >> "llvm.coro.raw.frame.ptr.addr" is conceptually "the address of a coroutine >> frame field storing the `raw frame pointer`" only after `insertSpills` in >> CoroFrame.cpp. Before that, "llvm.coro.raw.frame.ptr.addr" is actually an >> alloca storing the `raw frame pointer` (try grepping "alloc.frame.ptr" in >> this review page). Using "llvm.coro.raw.frame.ptr.offset" instead of >> "llvm.coro.raw.frame.ptr.addr" is doable which looks like below, please >> check line 31. The downside is that the write to coroutine frame is not >> through an alloca but a direct write. It is unusual because all fields in >> the frame are stored as 1. special/header fields 2. alloca 3. splills. Doing >> the write indirectly as Alloca makes me comfortable. The tradeoff is one >> extra intrinsic "llvm.coro.raw.frame.ptr.addr". What do you think? >> >> 19 coro.alloc.align: ; preds = >> %coro.alloc.check.align >> 20 %3 = sub nsw i64 64, 16 >> 21 %4 = add i64 128, %3 >> 22 %call1 = call noalias nonnull i8* @_Znwm(i64 %4) #13 >> 23 %mask = sub i64 64, 1 >> 24 %intptr = ptrtoint i8* %call1 to i64 >> 25 %over_boundary = add i64 %intptr, %mask >> 26 %inverted_mask = xor i64 %mask, -1 >> 27 %aligned_intptr = and i64 %over_boundary, %inverted_mask >> 28 %diff = sub i64 %aligned_intptr, %intptr >> 29 %aligned_result = getelementptr inbounds i8, i8* %call1, i64 %diff >> 30 call void @llvm.assume(i1 true) [ "align"(i8* %aligned_result, i64 >> 64) ] >> 31 store i8* %call1, i8** %alloc.frame.ptr, align 8 >> >> ; Replace line 31 with below, and must makes sure line 46~line 48 is >> skipped. >> ; %poff = call i32 @llvm.coro.raw.frame.ptr.offset.i32() >> ; %addr = getelementptr inbounds i8, i8* %aligned_result, i32 %poff >> ; %addr1 = bitcast i8* %addr to i8** >> ; store i8* %call1, i8** %addr1, align 8 >> >> >> 32 br label %coro.init.from.coro.alloc.align >> 33 >> 34 coro.init.from.coro.alloc.align: ; preds = >> %coro.alloc.align >> 35 %aligned_result.coro.init = phi i8* [ %aligned_result, >> %coro.alloc.align ] >> 36 br label %coro.init >> 37 >> 38 coro.init: ; preds = >> %coro.init.from.entry, %coro.init.from.coro.alloc.align, %cor >> o.init.from.coro.alloc >> 39 %5 = phi i8* [ %.coro.init, %coro.init.from.entry ], [ >> %call.coro.init, %coro.init.from.coro.alloc ], [ %aligned_result >> .coro.init, %coro.init.from.coro.alloc.align ] >> 40 %FramePtr = bitcast i8* %5 to %f0.Frame* >> 41 %resume.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 0 >> 42 store void (%f0.Frame*)* @f0.resume, void (%f0.Frame*)** >> %resume.addr, align 8 >> 43 %6 = select i1 true, void (%f0.Frame*)* @f0.destroy, void >> (%f0.Frame*)* @f0.cleanup >> 44 %destroy.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 1 >> 45 store void (%f0.Frame*)* %6, void (%f0.Frame*)** %destroy.addr, align >> 8 >> 46 %7 = getelementptr inbounds %f0.Frame, %f0.Frame* %FramePtr, i32 0, >> i32 2 >> 47 %8 = load i8*, i8** %alloc.frame.ptr, align 8 >> 48 store i8* %8, i8** %7, align 8 >> 49 br label %AllocaSpillBB >> 50 >> 51 AllocaSpillBB: ; preds = %coro.init >> 52 %.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 4 >> 53 %ref.tmp.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 5 >> 54 %agg.tmp.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 6 >> 55 %ref.tmp5.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 7 >> 56 %agg.tmp8.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 8 >> 57 %__promise.reload.addr = getelementptr inbounds %f0.Frame, %f0.Frame* >> %FramePtr, i32 0, i32 10 >> 58 br label %PostSpill >> >> >> >> >>> Then I am a little confused for the design again, since we would treat the >>> value for CoroBegin as the address of coroutine frame in the past and it >>> looks like to be the raw frame now. Let me reconsider if it is OK. >> >> The returned value of CoroBegin is still coroutine frame not a raw frame >> even if the frame is overaligned. You could check the above code. > > Thanks for clarifying! > > I don't understand why we need to store the address for coroutine raw frame > in the coroutine frame. For example, `%call1` in your example marks the > address for the raw frame. Then can we use the value `%call1` in every place > where we want to use the address for coroutine frame? > If yes, I think we could emit an intrinsic called 'llvm.coro.raw.frame' in > the frontend if we need to use the address for the raw frame. Then in the > middle end, we could replace `llvm.coro.raw.frame` with `%call1` simply. > Similarly, we could define intrinsic `llvm.coro.raw.frame.size`. As far as I > know from the codes, the address for the coroutine frame is mainly used for > deallocation. So it should be fine I guess. > > --- > > Then the code generated now looks roughly like: > > if (should over align) { > /// ... > mem = ... > } else { > /// ... > mem = ... > } > coro.begin(id, mem); > > It looks redundant since the `then` part and `else` part looks very similar. > I understand it would be eliminated in the middle end. But another problem is > that the redundant implementation in clang. Maybe we could solve it by > refactoring. > But I am wondering if it is possible to use another pattern (assume > `llvm.coro.alloc` returns true): > > %raw.frame.ptr = new(call @llvm.coro.raw.frame.size()) > %true.frame.ptr = call @llvm.coro.frame(%raw.frame.ptr, NEW_ALIGN) ; we > need a better name > call @llvm.coro.begin(coro.id, %true.frame.ptr) > > Then for `llvm.coro.frame`, we could return `@raw.frame.ptr ` simply if the > alignment could be satisfied (alignment needed is less than NEW_ALIGN). Or we > could do simply to align up for the coroutine frame. There are many APIs in > Align.h. > And for the destruction, we could emit: > > call @delete(%raw.frame.ptr, call @llvm.coro.raw.frame.size()) > > In this way, I guess we would get simpler implementation and generated codes. > > BTW, if we choose to do so, the semantics for llvm.coro.raw.frame.ptr and > llvm.coro.size would change slightly. They would stands for the address and > size for the coroutine frame if we don't need over alignment. > > How do you think about this? I was confused by this and @rjmccall explained it here https://reviews.llvm.org/D97915/new/#2604871. Basically, we could not recover "raw frame pointer" (`%call1`) from coroutine frame pointer statically at deallocation time. ================ Comment at: llvm/docs/Coroutines.rst:1087 The second argument is a pointer to the coroutine frame. This should be the same pointer that was returned by prior `coro.begin` call. ---------------- ChuanqiXu wrote: > > the coroutine frame > > It should be the raw frame now, isn't it? It is still "coroutine frame". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97915/new/ https://reviews.llvm.org/D97915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
