ychen added a comment. In D97915#2863581 <https://reviews.llvm.org/D97915#2863581>, @ChuanqiXu wrote:
> In D97915#2862419 <https://reviews.llvm.org/D97915#2862419>, @ychen wrote: > >>> It looks not hard to implement. And we don't need to refactor the CodeGen >>> part a lot. In this way, I think the main effort to support `::operator >>> new(size_t, align_t)` would be in the Sema part and the works remained in >>> CodeGen part would be little. It wouldn't touch the middle end part neither. >> >> I agree that something like this is simpler implementation-wise. Although in >> spirit, it is similar to D100739 <https://reviews.llvm.org/D100739> which we >> decided not to pursue. What is the middle-end? is it LLVM? >> >>>> It shifts the semantic lowering from Clang to LLVM but does not perform >>>> less work. >>> >>> I think it would be simpler. >> >> I agree it would be simpler. >> >>> At least, we don't need to emit `getReturnStmtOnAllocFailure` twice and >> >> `getReturnStmtOnAllocFailure` is called twice but the code is emitted once. >> >>> we don't need to touch `CallCoroDelete` neither. >> >> If we don't touch `CallCoroDelete`, then we cannot do the equivalent work in >> LLVM since there is no concept of deallocation function concept there. >> >>> And we don't organize the basic blocks in the CodeGenCoroutineBody. >> >> That's true. It is delayed until CoroSplit. >> >>> And we could emit simpler AlignupTo (Although it could be simplified >>> further, I believe). >> >> D102147 <https://reviews.llvm.org/D102147> has a version of it. >> >>> And the extra work we need to do is to compare the alignment requirement >>> for the coroutine frame with the second argument of >>> `llvm.coro.create.frame` to see if we need to over align coroutine frame. >>> If yes, we need to lower the `llvm.coro.create.frame` to compute the true >>> address for the coroutine frame and store the raw frame address. >>> If no, we could return `%allocated` simply. >> >> Yes, it is similar to CoroFrame.cpp changes in D102147 >> <https://reviews.llvm.org/D102147>. > > > >> I agree that something like this is simpler implementation-wise. Although in >> spirit, it is similar to D100739 <https://reviews.llvm.org/D100739> which we >> decided not to pursue. What is the middle-end? is it LLVM? > > It is frustrating to break the decision made before. But I don't find the > conclusion in D100739 <https://reviews.llvm.org/D100739> that we shouldn't > complete this in middle end. Yeah, it is not explicitly stated there. That's just my conclusion based on @rjmccall's suggestion (https://reviews.llvm.org/D100739#2717582) and my following responses. I do think your proposal works for the dynamic allocation cases, however, it needs major change when aligned allocator/deallocator comes into play in the future when the issue is fixed at the language level. That is the primary reason that most of the implementations are in front-end and LLVM coroutine intrinsics are mostly agnostic of the alignment issue (newly added llvm.coro.raw.frame.ptr.* are two exceptions and all existing intrinsics are not touched in both in API or semantics). >> `getReturnStmtOnAllocFailure` is called twice but the code is emitted once. > > I don't think so. `getReturnStmtOnAllocFailure` is called twice and emitted > twice. The code emitted by the frontend should be: > > AllocBB: > ; ... > getReturnStmtOnAllocFailure() > > AlignedAllocBB: > ; ... > getReturnStmtOnAllocFailure() > > I understand that one of the branch would be pruned in the middle end. But in > the frontend, they are emitted twice. And it's redundant in both compiler > codes and generated codes. Got you. I meant alloc failure block is emitted once and agree that both aligned and normal alloc block are emitted. >> If we don't touch CallCoroDelete, then we cannot do the equivalent work in >> LLVM since there is no concept of deallocation function concept there. > > Sorry for confusing. I means that we could change semantics for > `llvm.coro.free` to return the address of raw frame and touch the Sema part > to pass `llvm.coro.raw.frame.size` as the second argument to deallocator. > It should be easy and clean to implement and we don't need to touch > `CallCoroDelete ` either. > >>> And we don't organize the basic blocks in the CodeGenCoroutineBody. >> >> That's true. It is delayed until CoroSplit. > > I don't think it is delayed. I think the task to organize the BBs would be > eliminated if we move the main work in the middle end. > I think if we move the work to select to over align or not, we could use the > same IR structure. I think it is simpler, cleaner and more natural. Agree. But like I said above, IMHO, that's a secondary goal here. LLVM better only deal with optimizations, not semantics. (but, yeah, coroutine is special but the principle still applies). >>> And we could emit simpler AlignupTo (Although it could be simplified >>> further, I believe). >> >> D102147 <https://reviews.llvm.org/D102147> has a version of it. >> >>> And the extra work we need to do is to compare the alignment requirement >>> for the coroutine frame with the second argument of >>> `llvm.coro.create.frame` to see if we need to over align coroutine frame. >>> If yes, we need to lower the `llvm.coro.create.frame` to compute the true >>> address for the coroutine frame and store the raw frame address. >>> If no, we could return `%allocated` simply. >> >> Yes, it is similar to CoroFrame.cpp changes in D102147 >> <https://reviews.llvm.org/D102147>. > > I guess you refer to the wrong revision. Since I don't find related things in > D102147 <https://reviews.llvm.org/D102147>. Sorry for the typo. It should be D100739 <https://reviews.llvm.org/D100739>. 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
