[PATCH] D115790: [Coroutines] Set presplit attribute in Clang and mlir

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:712
+  assert(CurFn);
+  CurFn->addFnAttr("coroutine.presplit", "0");
 }
----------------
The assertion here is not necessary; if it was, we'd need it everywhere.

Please add a comment like "LLVM expects coroutines to have this attribute set coming out of the frontend."


================
Comment at: llvm/docs/Coroutines.rst:1179
+The frontend should add attribute `"coroutine.presplit"` with value `"0"` for the coroutine
+containing `coro.id`.
+
----------------
Please add this paragraph (modified appropriately) to all of the other `coro.id.*` intrinsics.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:198
       case Intrinsic::coro_id_async:
         F.addFnAttr(CORO_PRESPLIT_ATTR, PREPARED_FOR_SPLIT);
         break;
----------------
This should turn into the same assertion as above.  (Swift will need a change, but that's Swift's problem, and I'll take care of it.)


================
Comment at: llvm/lib/Transforms/Coroutines/CoroInternal.h:42
+// attributes since these attributes are already used outside the LLVM's
+// coroutine module.
 #define CORO_PRESPLIT_ATTR "coroutine.presplit"
----------------
Editorial: don't put "the" before LLVM here

Since this change requires frontend cooperation, and the refactor will *also* require frontend cooperation, should we just do this in one step so that frontends are less disrupted?


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

https://reviews.llvm.org/D115790