[PATCH] D22998: [coroutines] Part 4a: Coroutine Devirtualization: Lower coro.resume and coro.destroy.
David Majnemer via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 31 08:15:32 PDT 2016
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
More information about the llvm-commits
mailing list