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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
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.
----------------
ychen wrote:
> 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)
hmmm, doesn't it conflict with the replies above? Or does it mean these intrinsics could be used generally by design but we just don't support them for other ABI now?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:820
+      FrameData.Allocas.emplace_back(
+          FramePtrAddr, DenseMap<Instruction *, llvm::Optional<APInt>>{}, true);
+    }
----------------
ychen wrote:
> 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.
So does it mean this code would only work correctly if D97915 applies? It is a little bit weird for me. I understand that it would be clear er if we split a patch into separate ones. But it would be odd if these patches depends on each other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102145