[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

Thu Apr 22 23:50:40 PDT 2021

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