[PATCH] D92706: [coroutine] should disable inline before calling coro split

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> I believe it's not worth doing just to be able to use one constant string in one place. 
> If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

Agreed that string is fine. There're many precedences where string is used in `getFnAttribute`/`hasFnAttribute` already, otherwise we will need to be consistent.



================
Comment at: llvm/include/llvm/IR/Function.h:272
+  /// Check if this function is a coroutine.
+  bool isCoroutine() const { return hasFnAttribute("coroutine.presplit"); }
+
----------------
What is the definition of a coroutine function here? The main coroutine function alone, or does that include split funclets? A function is consider a coroutine function only before corosplit, but no longer after split? 

I'm not sure if presplit attribute is the best way to tell whether function is coroutine because the attribute is added, removed in the process of codegen only to reflect split state. And it doesn't look like an invariant. Also what we need here in the context of inline check is not whether a function is coroutine, but wether a function is a presplit coroutine, right?



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2339
+  // So we won't inline the coroutine function if it have not been unsplited
+  if (Callee->isCoroutine())
+    return InlineResult::failure("unsplited coroutine call");
----------------
This works with the current definition of isCoroutine which is based on split. But the naming is misleading because it may lead people to think that we can't inline any coroutine. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92706