[PATCH] D102145: [Coroutines] Add `llvm.coro.align`, `llvm.coro.raw.frame.ptr.offset` and `llvm.coro.raw.frame.ptr.addr` intrinsics

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

Thanks for the review!



================
Comment at: llvm/docs/Coroutines.rst:964
+The '``llvm.coro.align``' intrinsic returns the alignment of the coroutine frame
+in bytes.
+
----------------
ChuanqiXu wrote:
> If this is only valid under switch-resume coroutines, we need to mark them in the doc.
It's general, i.e not specific to switch-resume coroutines.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:796-825
+    Align FrameAlign =
+        std::max_element(
+            B.getFields().begin(), B.getFields().end(),
+            [](auto &F1, auto &F2) { return F1.Alignment < F2.Alignment; })
+            ->Alignment;
+
+    // Check for over-alignment.
----------------
ChuanqiXu wrote:
> If the new added intrinsic is intended for all APIs, we should move them out of the if blocks.
It is only supported for switched-resume coroutines. (mentioned in the doc)


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:820
+      FrameData.Allocas.emplace_back(
+          FramePtrAddr, DenseMap<Instruction *, llvm::Optional<APInt>>{}, true);
+    }
----------------
ChuanqiXu wrote:
> If we add a new alloca simply, it shouldn't work properly. For example,
> ```
> void foo() {
>      %a = alloca i32
>      %b = alloca i32
>      %promise = alloca PromiseType 
> }
> ```
> 
> The frame we want may be:
> ```
> %FrameType = type { [i8 x num of padding], resume_fn, destroy_fn, %promise, i32, i32, index}
> ```
> 
> However, if we simply add a new alloca with type i8*, we may get the FrameType with:
> ```
> %FrameType = type { resume_fn, destroy_fn, %promise, i32, i32, i32*, index}
> ```
> 
> I don't think this could solve our problems. Or does it work only with the successor patches?
Yes, following patch D97915 would do the overallocate&adjust frame start address. Here checks if  (1) the frame is overaligned and (2) if llvm.coro.raw.frame.ptr.* intrinsics are used. If both are true, create an alloca that goes to the frame. `coro-frame-overalign.ll` test has the details.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102145/new/

https://reviews.llvm.org/D102145