[PATCH] D22998: [coroutines] Part 4a: Coroutine Devirtualization: Lower coro.resume and coro.destroy.

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:28-31
@@ +27,6 @@
+  void lowerResumeOrDestroy(CallSite CS, CoroSubFnInst::ResumeKind);
+  Lowerer(Module& M) : LowererBase(M) {}
+public:
+  static std::unique_ptr<Lowerer> createIfNeeded(Module& M);
+  bool lowerEarlyIntrinsics(Function& F);
+};
----------------
Please clang-format this, pointers and refs lean right.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:41-42
@@ +40,4 @@
+                                   CoroSubFnInst::ResumeKind Index) {
+  Value *ResumeAddr =
+    makeSubFnCall(CS.getArgOperand(0), Index, CS.getInstruction());
+  CS.setCalledFunction(ResumeAddr);
----------------
This doesn't look clang-format'd.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:44
@@ +43,3 @@
+  CS.setCalledFunction(ResumeAddr);
+  CS.setCallingConv(CallingConv::Fast);
+}
----------------
Why do we change the calling convention?

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:47
@@ +46,3 @@
+
+bool Lowerer::lowerEarlyIntrinsics(Function& F) {
+  bool changed = false;
----------------
clang-format this, pointers and refs lean right.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:48
@@ +47,3 @@
+bool Lowerer::lowerEarlyIntrinsics(Function& F) {
+  bool changed = false;
+  for (auto IB = inst_begin(F), IE = inst_end(F); IB != IE;) {
----------------
Variables start with caps.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:51
@@ +50,3 @@
+    Instruction &I = *IB++;
+    if (auto II = dyn_cast<IntrinsicInst>(&I)) {
+      switch (II->getIntrinsicID()) {
----------------
I'd suggest `auto *` to make it clear we are copying around a pointer.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:70
@@ +69,3 @@
+// the module.
+std::unique_ptr<Lowerer> Lowerer::createIfNeeded(Module& M) {
+  if (M.getNamedValue(CORO_RESUME_STR) || M.getNamedValue(CORO_DESTROY_STR))
----------------
Please clang-format this.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:71
@@ +70,3 @@
+std::unique_ptr<Lowerer> Lowerer::createIfNeeded(Module& M) {
+  if (M.getNamedValue(CORO_RESUME_STR) || M.getNamedValue(CORO_DESTROY_STR))
+    return std::unique_ptr<Lowerer>(new Lowerer(M));
----------------
Could you use the llvm_coro_{destroy,resume} intrinsic IDs instead?

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:72
@@ +71,3 @@
+  if (M.getNamedValue(CORO_RESUME_STR) || M.getNamedValue(CORO_DESTROY_STR))
+    return std::unique_ptr<Lowerer>(new Lowerer(M));
+
----------------
LLVM has a `make_unique` implementation.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:74
@@ +73,3 @@
+
+  return{};
+}
----------------
Please clang-format this.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:89
@@ +88,3 @@
+
+  bool doInitialization(Module& M) override {
+    L = Lowerer::createIfNeeded(M);
----------------
The reference should lean right.

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:1
@@ +1,2 @@
+//===-- llvm/CoroInst.h - Coroutine Intrinsics Wrappers ---------*- C++ -*-===//
+//
----------------
The file path doesn't match the file, I'd just make it "CoroInstr.h"

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:36
@@ +35,3 @@
+class LLVM_LIBRARY_VISIBILITY CoroSubFnInst : public IntrinsicInst {
+  enum { kFrame, kIndex };
+public:
----------------
We don't use this naming style for enums, see [[ http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | here ]]

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:47
@@ +46,3 @@
+  ResumeKind getIndex() const {
+    auto Index = getRawIndex()->getValue().getSExtValue();
+    assert(Index >= IndexFirst && Index < IndexLast &&
----------------
I'd make this `auto` just `int64_t`.

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:53
@@ +52,3 @@
+
+  ConstantInt* getRawIndex() const {
+    return cast<ConstantInt>(getArgOperand(kIndex));
----------------
clang-format this.

================
Comment at: lib/Transforms/Coroutines/CoroInternal.h:20-23
@@ -18,3 +19,6 @@
 
 class PassRegistry;
+class Module;
+class LLVMContext;
+class FunctionType;
 
----------------
Please sort these.

================
Comment at: lib/Transforms/Coroutines/CoroInternal.h:34-36
@@ +33,5 @@
+struct LowererBase {
+  Module& M;
+  LLVMContext& C;
+  FunctionType* const ResumeFnType;
+
----------------
Pointers lean right.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:70-74
@@ +69,7 @@
+
+// Construct the lowerer base class and initialize its members.
+coro::LowererBase::LowererBase(Module &M)
+  : M(M), C(M.getContext()),
+  ResumeFnType(FunctionType::get(Type::getVoidTy(C), Type::getInt8PtrTy(C),
+    /*isVarArg=*/false)) {}
+
----------------
Please clang-format this.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:82-93
@@ +81,14 @@
+
+Value *coro::LowererBase::makeSubFnCall(Value *Arg, int Index,
+  Instruction *InsertPt) {
+  auto IndexVal = ConstantInt::get(Type::getInt8Ty(C), Index);
+  auto Fn = Intrinsic::getDeclaration(&M, Intrinsic::coro_subfn_addr);
+
+  SmallVector<Value *, 2> Args{ Arg, IndexVal };
+  auto Call = CallInst::Create(Fn, Args, "", InsertPt);
+
+  auto Bitcast =
+    new BitCastInst(Call, ResumeFnType->getPointerTo(), "", InsertPt);
+  return Bitcast;
+}
+
----------------
This doesn't look clang-format'd.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:84
@@ +83,3 @@
+  Instruction *InsertPt) {
+  auto IndexVal = ConstantInt::get(Type::getInt8Ty(C), Index);
+  auto Fn = Intrinsic::getDeclaration(&M, Intrinsic::coro_subfn_addr);
----------------
majnemer wrote:
> We prefer `auto *` to make it clear we are copying around pointers.
Should we assert that `Index` is `>= 0`?

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:84-92
@@ +83,11 @@
+  Instruction *InsertPt) {
+  auto IndexVal = ConstantInt::get(Type::getInt8Ty(C), Index);
+  auto Fn = Intrinsic::getDeclaration(&M, Intrinsic::coro_subfn_addr);
+
+  SmallVector<Value *, 2> Args{ Arg, IndexVal };
+  auto Call = CallInst::Create(Fn, Args, "", InsertPt);
+
+  auto Bitcast =
+    new BitCastInst(Call, ResumeFnType->getPointerTo(), "", InsertPt);
+  return Bitcast;
+}
----------------
We prefer `auto *` to make it clear we are copying around pointers.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:87-88
@@ +86,4 @@
+
+  SmallVector<Value *, 2> Args{ Arg, IndexVal };
+  auto Call = CallInst::Create(Fn, Args, "", InsertPt);
+
----------------
I think you can just sink the args list right into the `CallInst::Create`, `ArrayRef` has a `std::initializer_list` constructor.


Repository:
  rL LLVM

https://reviews.llvm.org/D22998