[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:820
+      FrameData.Allocas.emplace_back(
+          FramePtrAddr, DenseMap<Instruction *, llvm::Optional<APInt>>{}, true);
+    }
----------------
ychen wrote:
> ChuanqiXu wrote:
> > 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.
> > So does it mean this code would only work correctly if D97915 applies?
> 
> No. This patch only lowers the new intrinsics under switched-resume ABI. Without D97915, there are no uses of these new intrinsics, so this patch basically no-op before D97915 applies.
It is still weird for me. It looks like these codes are redundant if we don't applies `D97915`. I am still prefer to merge this with `D97915`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102145