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

[Gor Nishanov via llvm-commits]

GorNishanov added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:44
@@ +43,3 @@
+  CS.setCalledFunction(ResumeAddr);
+  CS.setCallingConv(CallingConv::Fast);
+}
----------------
majnemer wrote:
> Why do we change the calling convention?
The coro.resume / coro.destroy intrinsics hide from the frontend details of the exact calling convention of post split coroutine subfunctions. LLVM knows that the extracted parts will have fastcc calling conventions (since we will be generating them in CoroSplit and will select fastcc calling convention in later patches).


================
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));
----------------
majnemer wrote:
> mehdi_amini wrote:
> > majnemer wrote:
> > > Could you use the llvm_coro_{destroy,resume} intrinsic IDs instead?
> > Can we find if an intrinsic is declared/used in a Module using the ID?
> Ah, maybe not...
Yep. M.getDeclaration(ID, Types) will call getOrInsertFunction that will insert a declaration if one was not available.

I was looking for least expensive check and was following an example of:

```
/// \brief Test if the given module looks interesting to run ARC optimization on
inline bool ModuleHasARC(const Module &M) {
  return
    M.getNamedValue("objc_retain") ||
    M.getNamedValue("objc_release") ||
```


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

It is a private enum inside of the class, so it does not need a prefix according to the coding guidelines.

I did want to have it stand out a little to signify that it is a constant. I considered

s/kFrame/FRAME
s/kIndex/INDEX ?

but then it maybe consumed with a macro.

================
Comment at: lib/Transforms/Coroutines/CoroInternal.h:35
@@ +34,3 @@
+  Module& M;
+  LLVMContext& C;
+  FunctionType* const ResumeFnType;
----------------
mehdi_amini wrote:
> `M` and `C` aren't great variable names. Also, is `C` used outside of the ctor? (The name does not help for grep...)
I was following examples of similar classes in other components, such as in FunctionImportUtils.h, WholeProgramDevirt.cpp and many others.

But, I will be happy to change them to:

  LLVMContext &Context;
  Module &TheModule; 

These also seem to be popular choices in the codebase.

Context is not used outside of the constructor in this patch. It will be in future patches. I can remove it for now.


Repository:
  rL LLVM

https://reviews.llvm.org/D22998