Re: [PATCH] D24373: [Coroutines] Adding builtins for coroutine intrinsics and backendutil support.

[Gor Nishanov via cfe-commits]

GorNishanov marked an inline comment as done.


Comment at: include/clang/Basic/Builtins.def:1293
@@ +1292,3 @@
+
+BUILTIN(__builtin_coro_id, "v*Iiv*v*v*", "n")
+BUILTIN(__builtin_coro_alloc, "bv*", "n")

rsmith wrote:
> I don't really like having builtins which will result in errors from the 
> middle-end in some cases; there are a bunch of side-conditions on 
> llvm.coro.id that aren't being enforced here. In particular, this call must 
> be present in any function that also calls coro.alloc and friends, and must 
> dominate those other calls.
> 
> Modeling the 'token' value as a `void*` also seems problematic. If the user 
> uses that value for anything other than the argument to a builtin that wants 
> the token, bad things are going to happen.
> 
> (From dinner discussion:) one possible way to deal with this would be to 
> generate the call to @llvm.coro.id implicitly in the entry block, in a 
> function that uses the other builtins. The challenge then is communicating 
> the promise object to the intrinsic, which is problematic if we allow an 
> arbitrary pointer value to be passed in.
> 
> However, we're only interested in supporting a stack variable as the promise 
> object, so here's one possible approach:
> 
> - add an attribute that can be applied to a local variable to mark it as the 
> promise object for the current function
> - remove the `__builtin_coro_id` builtin, and instead implicitly generate the 
> `llvm.coro.id` intrinsic call in the entry block when we need its token or 
> see a promise object
> - when we emit a local variable with the promise-object attribute, update the 
> `llvm.coro.id` call to pass its alloca as the promise object
> - remove the 'token' parameter from `__builtin_coro_alloc` etc, and instead 
> implicitly provide it from the result of the implicit `llvm.coro.id` call
I added clarification to the documentation that all but four builtins are for 
internal compiler use and for the use as development tools for the coroutine 
feature, so, possibly we should not worry too much about people misusing them.

Alternatively, I can get rid of most of the coroutine builtins, apart from the 
ones that are intended to be used to implement coroutine standard library 
facilities.

At the moment, I think, we should prioritize getting C++ Coroutines online. We 
can polish C coroutine story later.


Comment at: test/Coroutines/coro.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines -emit-llvm %s 
-o - -O3 | FileCheck %s
+#include "Inputs/coro.h"

rsmith wrote:
> Please just check the IR generated by the frontend is correct for each of the 
> intrinsics rather than using an end-to-end test that depends on the LLVM IR 
> optimizations. You can use `-disable-llvm-optzns` to see the IR coming out of 
> clang before the mandatory coroutine passes monkey with it.
I added two tests to check that builtins are lowered to coro intrinsics 
correctly. I would like to keep a small number of "-O2" tests as a sanity 
end-to-end testing. 


https://reviews.llvm.org/D24373



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24373: [Coroutines] Adding builtins for coroutine intrinsics and backendutil support.

[Gor Nishanov via cfe-commits]

GorNishanov updated the summary for this revision.
GorNishanov removed a reviewer: majnemer.
GorNishanov updated this revision to Diff 72732.
GorNishanov added a comment.
Herald added a subscriber: mgorny.

1. Added documentation for builtins
2. Added a couple of tests with -disable-llvm-passes to check that builtins are 
emitted correctly


https://reviews.llvm.org/D24373

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Builtins.def
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCoroutine.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenCoroutines/Inputs/coro.h
  test/CodeGenCoroutines/O2-coro.c
  test/CodeGenCoroutines/coro-builtins-err.c
  test/CodeGenCoroutines/coro-builtins.c

Index: test/CodeGenCoroutines/coro-builtins.c
===
--- /dev/null
+++ test/CodeGenCoroutines/coro-builtins.c
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc18.0.0 -fcoroutines -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+
+// CHECK-LABEL: f( 
+void f() {
+  // CHECK: %0 = call token @llvm.coro.id(i32 32, i8* null, i8* null, i8* null)
+  __builtin_coro_id(32, 0, 0, 0);
+  // CHECK-NEXT: %1 = call i1 @llvm.coro.alloc(token %0)
+  __builtin_coro_alloc();
+  // CHECK-NEXT: %2 = call i64 @llvm.coro.size.i64()
+  __builtin_coro_size();
+  // CHECK-NEXT: %3 = call i8* @llvm.coro.begin(token %0, i8* null)
+  __builtin_coro_begin(0);
+  // CHECK-NEXT: %4 = call i8* @llvm.coro.frame() 
+  __builtin_coro_frame();
+  // CHECK-NEXT: call void @llvm.coro.resume(i8* null)
+  __builtin_coro_resume(0);
+  // CHECK-NEXT: call void @llvm.coro.destroy(i8* null)
+  __builtin_coro_destroy(0);
+  // CHECK-NEXT: %5 = call i1 @llvm.coro.done(i8* null)
+  __builtin_coro_done(0);
+  // CHECK-NEXT: %6 = call i8* @llvm.coro.promise(i8* null, i32 32, i1 false)
+  __builtin_coro_promise(0, 32, 0);
+  // CHECK-NEXT: %7 = call i8* @llvm.coro.free(token %0, i8* null)
+  __builtin_coro_free(0);
+  // CHECK-NEXT: call void @llvm.coro.end(i8* null, i1 false)
+  __builtin_coro_end(0, 0);
+  // CHECK-NEXT: %8 = call i8 @llvm.coro.suspend(token none, i1 false)
+  __builtin_coro_suspend(0);
+  // CHECK-NEXT: %9 = call i1 @llvm.coro.param(i8* null, i8* null)
+  __builtin_coro_param(0, 0);
+}
Index: test/CodeGenCoroutines/coro-builtins-err.c
===
--- /dev/null
+++ test/CodeGenCoroutines/coro-builtins-err.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc18.0.0 -fcoroutines -emit-llvm %s -o - -verify
+
+void f() {
+  __builtin_coro_alloc(); // expected-error {{this builtin expect that __builtin_coro_id}}
+  __builtin_coro_begin(0); // expected-error {{this builtin expect that __builtin_coro_id}}
+  __builtin_coro_free(0); // expected-error {{this builtin expect that __builtin_coro_id}}
+
+  __builtin_coro_id(32, 0, 0, 0);
+  __builtin_coro_id(32, 0, 0, 0); // expected-error {{only one __builtin_coro_id}}
+}
Index: test/CodeGenCoroutines/O2-coro.c
===
--- /dev/null
+++ test/CodeGenCoroutines/O2-coro.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines -emit-llvm %s -o - -O3 | FileCheck %s
+#include "Inputs/coro.h"
+void print(int);
+
+void* f() {
+  CORO_BEGIN(malloc);
+
+  for (int i = 0;; ++i) {
+print(i);
+CORO_SUSPEND();
+  }
+
+  CORO_END(free);
+}
+
+// CHECK-LABEL: @main
+int main() {
+  void* coro = f();
+  CORO_RESUME(coro);
+  CORO_RESUME(coro);
+  CORO_DESTROY(coro);
+// CHECK: call void @print(i32 0)
+// CHECK: call void @print(i32 1)
+// CHECK: call void @print(i32 2)
+}
Index: test/CodeGenCoroutines/Inputs/coro.h
===
--- /dev/null
+++ test/CodeGenCoroutines/Inputs/coro.h
@@ -0,0 +1,37 @@
+void free(void *ptr);
+void *malloc(unsigned int);
+
+#define CORO_SUSPEND_IMPL(IsFinal) \
+  switch (__builtin_coro_suspend(IsFinal)) {   \
+  case 0:  \
+if (IsFinal)   \
+  __builtin_trap();\
+break; \
+  case 1:  \
+goto coro_Cleanup; \
+  default: \
+goto coro_Suspend; \
+  }
+
+#define CORO_SUSPEND() CORO_SUSPEND_IMPL(0)
+#define CORO_FINAL_SUSPEND() CORO_SUSPEND_IMPL(1)
+
+#define CORO_BEGIN(AllocFunc)  \
+  void *coro_hdl = 

Re: [PATCH] D24373: [Coroutines] Adding builtins for coroutine intrinsics and backendutil support.

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: include/clang/Basic/Builtins.def:1293
@@ +1292,3 @@
+
+BUILTIN(__builtin_coro_id, "v*Iiv*v*v*", "n")
+BUILTIN(__builtin_coro_alloc, "bv*", "n")

I don't really like having builtins which will result in errors from the 
middle-end in some cases; there are a bunch of side-conditions on llvm.coro.id 
that aren't being enforced here. In particular, this call must be present in 
any function that also calls coro.alloc and friends, and must dominate those 
other calls.

Modeling the 'token' value as a `void*` also seems problematic. If the user 
uses that value for anything other than the argument to a builtin that wants 
the token, bad things are going to happen.

(From dinner discussion:) one possible way to deal with this would be to 
generate the call to @llvm.coro.id implicitly in the entry block, in a function 
that uses the other builtins. The challenge then is communicating the promise 
object to the intrinsic, which is problematic if we allow an arbitrary pointer 
value to be passed in.

However, we're only interested in supporting a stack variable as the promise 
object, so here's one possible approach:

- add an attribute that can be applied to a local variable to mark it as the 
promise object for the current function
- remove the `__builtin_coro_id` builtin, and instead implicitly generate the 
`llvm.coro.id` intrinsic call in the entry block when we need its token or see 
a promise object
- when we emit a local variable with the promise-object attribute, update the 
`llvm.coro.id` call to pass its alloca as the promise object
- remove the 'token' parameter from `__builtin_coro_alloc` etc, and instead 
implicitly provide it from the result of the implicit `llvm.coro.id` call


Comment at: test/Coroutines/coro.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines -emit-llvm %s 
-o - -O3 | FileCheck %s
+#include "Inputs/coro.h"

Please just check the IR generated by the frontend is correct for each of the 
intrinsics rather than using an end-to-end test that depends on the LLVM IR 
optimizations. You can use `-disable-llvm-optzns` to see the IR coming out of 
clang before the mandatory coroutine passes monkey with it.


https://reviews.llvm.org/D24373



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits