ychen added a comment. In D100739#2706973 <https://reviews.llvm.org/D100739#2706973>, @lxfind wrote:
> Thanks for working on this. > I am still having a bit hard time understanding the solution. > A few questions: > > 1. I assume this patch is to solve the problem where the promise object is > not aligned according to its alignof annotation, right? The title/wording is > a bit misleading. Usually "handling XXX" means XXX is a situation/problem > that wasn't handle properly before, and it's being handled here. I don't > really understand what "handle overaligned frame allocation" means. Isn't > frame allocation under-aligned being the problem? Sorry for the confusion. I think either overaligned or under-aligned could be used here to describe the problem: either "Handle overaligned frame" or "Fix under-aligned frame". Since c++ spec defines the former but not the later (https://en.cppreference.com/w/cpp/language/object#Alignment), my first intuition was to use the term "overalign". Under-aligned is the undesired outcome that should be fixed (probably too late to handle I assume). Also the overaligned is a static property whereas 'under-aligned" is a runtime property. From the compiler's perspective, I think overaligned should be preferred. With that said, I don't feel strongly about this. I could switch to use "overaligned" if that feels more intuitive. > 2. What is the purpose of coro.align intrinsic? To communicate frame alignment to the frontend. It shouldn't be needed for this patch, so I've removed it. > 3. Could you provide some examples of what the IR might look like after this > patch? Either that or a more detailed explanation of how this works in the > summary. Yep, please see the updated description. And a new test is added. > 4. Do you think it might be cleaner to introduce a new variant of coro.size > instead of adding arguments to it? For example, coro.size.aligned(). This > way, you can avoid changing any test file for non-switch-lowering test files, > but focus on all switch-lowering tests. Agree, I've thought about variadic intrinsic and this new intrinsic, I think using new intrinsic is more flexible and avoids test fixup. > 5. Typically, coro.free is used by a comparison with nullptr. This is to > enable CoroElide. See: > https://llvm.org/docs/Coroutines.html#llvm-coro-free-intrinsic. So I don't > think you can load from it directly. Agree, I've changed to do it in `coro::replaceCoroFree`. @ChuanqiXu @lxfind Thanks a lot for the feedback. I've updated the description and addressed the existing comments. Please take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
