[PATCH] D23993: [Coroutines] Part 10: Add coroutine promise support.

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:66
@@ +65,3 @@
+  int64_t Alignement = Intrin->getAlignment();
+  Type *Int8Ty = Type::getInt8Ty(Context);
+
----------------
`Builder->getInt8Ty()` is a little shorter.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:68
@@ +67,3 @@
+
+  auto SampleStruct =
+      StructType::get(Context, {AnyResumeFnPtrTy, AnyResumeFnPtrTy, Int8Ty});
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:104-105
@@ +103,4 @@
+      return;
+    assert((isa<BitCastInst>(Arg) || isa<GetElementPtrInst>(Arg)) &&
+           "unexpected instruction designating the promise");
+    // TODO: Add a check that any remaining users of Inst are after coro.begin
----------------
This is weird.  There can be `ptrtoint`/`inttoptr` pairs, etc.  Why do you have this restriction?

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:108
@@ +107,3 @@
+    // or add code to move the users after coro.begin.
+    auto Inst = cast<Instruction>(Arg);
+    if (Inst->use_empty()) {
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:111-112
@@ +110,4 @@
+      Inst->eraseFromParent();
+      return;
+    } else
+      Inst->moveBefore(getCoroBegin()->getNextNode());
----------------
Don't use else after return: http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

================
Comment at: lib/Transforms/Coroutines/CoroInstr.h:242-243
@@ +241,4 @@
+  }
+  int64_t getAlignment() const {
+    return cast<ConstantInt>(getArgOperand(AlignArg))->getSExtValue();
+  }
----------------
The alignment is signed?


https://reviews.llvm.org/D23993