[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/docs/Coroutines.rst:964
+The '``llvm.coro.align``' intrinsic returns the alignment of the coroutine frame
+in bytes.
+
----------------
If this is only valid under switch-resume coroutines, we need to mark them in the doc.


================
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.
----------------
If the new added intrinsic is intended for all APIs, we should move them out of the if blocks.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:820
+      FrameData.Allocas.emplace_back(
+          FramePtrAddr, DenseMap<Instruction *, llvm::Optional<APInt>>{}, true);
+    }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102145