ChuanqiXu added inline comments.
================ Comment at: clang/include/clang/AST/StmtCXX.h:356-359 Expr *Allocate = nullptr; Expr *Deallocate = nullptr; + Expr *AlignedAllocate = nullptr; + Expr *AlignedDeallocate = nullptr; ---------------- Can't we merge these? ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:445-483 +void emitDynamicAlignedDealloc(CodeGenFunction &CGF, + llvm::BasicBlock *AlignedFreeBB, + llvm::CallInst *CoroFree) { + llvm::CallInst *Dealloc = nullptr; + for (llvm::User *U : CoroFree->users()) { + if (auto *CI = dyn_cast<llvm::CallInst>(U)) + if (CI->getParent() == CGF.Builder.GetInsertBlock()) ---------------- This code would only work if we use `::operator new(size_t, align_val_t)`, which is implemented in another patch. I would suggest to move this into that one. ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:528-547 CGF.EmitBlock(FreeBB); CGF.EmitStmt(Deallocate); - - auto *AfterFreeBB = CGF.createBasicBlock("after.coro.free"); - CGF.EmitBlock(AfterFreeBB); + CGF.Builder.CreateBr(AfterFreeBB); // We should have captured coro.free from the emission of deallocate. auto *CoroFree = CGF.CurCoro.Data->LastCoroFree; + CGF.CurCoro.Data->LastCoroFreeUsedForDealloc = true; ---------------- It looks like it would emit a `deallocate` first, and emit an `alignedDeallocate`, which is very odd. Although I can find that the second `deallocate` wouldn't be emitted due to the check `LastCoroFreeUsedForDealloc`, it is still very odd to me. If the second `deallocate` wouldn't come up all the way, what's the reason we need to write `emit(deallocate)` twice? ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:700 + auto *AlignedAllocateCall = EmitScalarExpr(S.getAlignedAllocate()); + bool HasAlignArg = hasAlignArg(cast<llvm::CallInst>(AlignedAllocateCall)); + ---------------- Since `hasAlignArg` is called only once, I suggested to make it a lambda here which would make the code more easy to read. ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:702-704 + if (!HasAlignArg) + overAllocateFrame(*this, cast<llvm::CallInst>(AlignedAllocateCall), + /*IsAlloc*/ true); ---------------- I recommend to add a detailed comment here to tell the story why we need to over allocate the frame. It is really hard to understand for people who are new to this code. Otherwise, I think they need to use `git blame` to find the commit id and this review page to figure the reasons out. ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:706-728 + if (auto *RetOnAllocFailure = S.getReturnStmtOnAllocFailure()) { + auto *Cond = Builder.CreateICmpNE(AlignedAllocateCall, NullPtr); + if (HasAlignArg) { + Builder.CreateCondBr(Cond, InitBB, RetOnFailureBB); + } else { + AlignAllocBB2 = createBasicBlock("coro.alloc.align2"); + Builder.CreateCondBr(Cond, AlignAllocBB2, RetOnFailureBB); ---------------- It may be better to organize it as: ``` if (!HasAlignArg) { if (auto *RetOnAllocFailure = S.getReturnStmtOnAllocFailure()) { auto *Cond = Builder.CreateICmpNE(AlignedAllocateCall, NullPtr); AlignAllocBB2 = createBasicBlock("coro.alloc.align2"); Builder.CreateCondBr(Cond, AlignAllocBB2, RetOnFailureBB); EmitBlock(AlignAllocBB2); } auto *CoroAlign = Builder.CreateCall( CGM.getIntrinsic(llvm::Intrinsic::coro_align, SizeTy)); ... } ``` ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:878 + if (CurCoro.Data && CurCoro.Data->LastCoroFreeUsedForDealloc) + return RValue::get(CurCoro.Data->LastCoroFree); + ---------------- Is it possible that it would return a nullptr value? 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