Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Ilya Verbin]

On Tue, Mar 29, 2016 at 17:15:11 +0200, Thomas Schwinge wrote:
> On Mon, 28 Mar 2016 19:40:22 +0300, Ilya Verbin  wrote:
> > Do you plan to commit this patch? :)
> 
> Well, I'm also still waiting for you guys to merge (via the upstream
> Intel sources repository) my GNU Hurd portability patches; submitted to
> GCC in
> 
> and the following messages, dated 2014-09-26.  Upon request of Barry M
> Tannenbaum then submitted to the Intel web site, and then never heard of
> again...  ;-(

I'm going to merge libcilkrts from upstream at stage1.  Your patch is there:
https://bitbucket.org/intelcilkruntime/intel-cilk-runtime/commits/2b33a7bfcbcd1def8108287475755b68b4aef2f7

  -- Ilya

Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Thomas Schwinge]

Hi!

On Mon, 28 Mar 2016 19:40:22 +0300, Ilya Verbin  wrote:
> Do you plan to commit this patch? :)

Well, I'm also still waiting for you guys to merge (via the upstream
Intel sources repository) my GNU Hurd portability patches; submitted to
GCC in

and the following messages, dated 2014-09-26.  Upon request of Barry M
Tannenbaum then submitted to the Intel web site, and then never heard of
again...  ;-(

> On Mon, Sep 29, 2014 at 09:24:40 -0600, Jeff Law wrote:
> > On 09/29/14 08:26, Thomas Schwinge wrote:
> > > Audit Cilk Plus tests for CILK_NWORKERS=1.
> > >
> > >   gcc/testsuite/
> > >   * c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
> > >   __cilkrts_set_param to set two workers.
> > >   * c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
> > >   * g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.

Thanks for reminding me about this.  I confirmed that the problem still
reproduces, and the very same patch still fixes it; now committed in
r234523:

commit 4abd94105ecb1d026406648a37ff2fb43bb26d7c
Author: tschwinge 
Date:   Tue Mar 29 14:39:33 2016 +

[PR testsuite/64177] Audit Cilk Plus tests for CILK_NWORKERS=1

PR testsuite/64177
gcc/testsuite/
* c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
__cilkrts_set_param to set two workers.
* c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
* g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@234523 
138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/testsuite/ChangeLog   |8 
 .../c-c++-common/cilk-plus/CK/spawning_arg.c  |   15 +++
 gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c |   17 ++---
 gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc|   14 ++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog
index 11d6863..f9b4b00 100644
--- gcc/testsuite/ChangeLog
+++ gcc/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2016-03-29  Thomas Schwinge  
+
+   PR testsuite/64177
+   * c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
+   __cilkrts_set_param to set two workers.
+   * c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
+   * g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.
+
 2016-03-28  Dominique d'Humieres  
 
g++.dg/ext/fnname5.C: Update the test for Darwin.
diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
index 95e6cab..138b82c 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
@@ -2,6 +2,17 @@
 /* { dg-options "-fcilkplus" } */
 /* { dg-additional-options "-lcilkrts" { target { i?86-*-* x86_64-*-* } } } */
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
+
 void f0(volatile int *steal_flag)
 { 
   int i = 0;
@@ -32,6 +43,10 @@ void f3()
 
 int main()
 {
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param("nworkers", "2") != 0)
+__builtin_abort();
+
   f3();
   return 0;
 }
diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
index 6e28765..6b41c7f 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
@@ -2,8 +2,16 @@
 /* { dg-options "-fcilkplus" } */
 /* { dg-additional-options "-lcilkrts" { target { i?86-*-* x86_64-*-* } } } */
 
-// #include 
-extern void __cilkrts_set_param (char *, char *);
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void foo(volatile int *);
 
@@ -11,7 +19,10 @@ void main2(void);
 
 int main(void)
 {
- //  __cilkrts_set_param ((char *)"nworkers", (char *)"2");
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param("nworkers", "2") != 0)
+__builtin_abort();
+
   main2();
   return 0;
 }
diff --git gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc 
gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
index 0633d19..09ddf8b 100644
--- gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
+++ gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
@@ -10,6 +10,16 @@
 #endif
 #include 
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void func(int volatile* steal_me) 
 {
@@ -59,6 +69,10 @@ void my_test()
 
 int main() 
 {
+  /* Ensure more than one worker.  */
+  if 

Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Ilya Verbin]

Hi Thomas!

Do you plan to commit this patch? :)

On Mon, Sep 29, 2014 at 09:24:40 -0600, Jeff Law wrote:
> On 09/29/14 08:26, Thomas Schwinge wrote:
> >On Mon, 29 Sep 2014 13:58:31 +, "Tannenbaum, Barry M" 
> > wrote:
> >>In a nutshell, add the following code to main() before the call to f3():
> >>
> >> int status = __cilkrts_set_param("nworkers", "2");
> >> if (0 != status) {
> >> // Failed to set the number of Cilk workers
> >> return status;
> >> }
> >
> >Yeah, that's what I had proposed with the patch at the end of my previous
> >email,
> >.
> >I'm sorry if I didn't make it obvious that more text and the patch were
> >following after the full-quote of the original issue description.
> >
> >>Here's the details: [...]
> >
> >Thanks again for your helpful comments; that's appreciated.
> >
> >Here's again my proposed patch.  Note, that the include paths in GCC
> >compiler testing (gcc/testsuite/) are not set up to pick up the
> > include file, so I've manually added a propotype for
> >the __cilkrts_set_param function to the three files.  I can change that,
> >if requested.
> >
> >commit ee7138e451d1f3284d6fa0f61fe517c82db94060
> >Author: Thomas Schwinge 
> >Date:   Mon Sep 29 12:47:34 2014 +0200
> >
> > Audit Cilk Plus tests for CILK_NWORKERS=1.
> >
> > gcc/testsuite/
> > * c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
> > __cilkrts_set_param to set two workers.
> > * c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
> > * g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.
> OK.
> Jeff

  -- Ilya

RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Thomas Schwinge]

Hi!

On Mon, 22 Sep 2014 19:21:33 +, Tannenbaum, Barry M 
barry.m.tannenb...@intel.com wrote:
 That's exactly correct.
 
 There are two ways to implement a work-stealing scheduler. We refer to them 
 as: [...]

Thanks for the explanation.

 Cilk implements Parent Stealing. Since you're running with 1 worker, there's 
 no other worker to steal the continuation. So steal_flag will never be set to 
 1 and you'll never break out of the loop.

Remains the question about how to address that in the testsuite:

 -Original Message-
 From: Thomas Schwinge [mailto:tho...@codesourcery.com] 
 Sent: Monday, September 22, 2014 9:56 AM
 To: Iyer, Balaji V
 Cc: gcc-patches@gcc.gnu.org
 Subject: Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
 Hi!
 
 On Tue, 27 Aug 2013 21:30:49 +, Iyer, Balaji V 
 balaji.v.i...@intel.com wrote:
  --- /dev/null
  +++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
  @@ -0,0 +1,37 @@
  +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
  +/* { dg-options -fcilkplus } */
  +/* { dg-options -lcilkrts { target { i?86-*-* x86_64-*-* arm*-*-* } 
  +} } */
  +
  +void f0(volatile int *steal_flag)
  +{
  +  int i = 0;
  +  /* Wait for steal_flag to be set */
  +  while (!*steal_flag) 
  +;
  +}
  +
  +int f1()
  +{
  +
  +  volatile int steal_flag = 0;
  +  _Cilk_spawn f0(steal_flag);
  +  steal_flag = 1;  // Indicate stolen
  +  _Cilk_sync;
  +  return 0;
  +}
  +
  +void f2(int q)
  +{
  +  q = 5;
  +}
  +
  +void f3()
  +{
  +   _Cilk_spawn f2(f1());
  +}
  +
  +int main()
  +{
  +  f3();
  +  return 0;
  +}
 
 Is this really well-formed Cilk Plus code?  Running with CILK_NWORKERS=1, or 
 -- the equivalent -- in a system with just one CPU (as per 
 libcilkrts/runtime/os-unix.c:__cilkrts_hardware_cpu_count returning 1), I see 
 this test busy-loop as follows:
 
 Breakpoint 1, __cilkrts_hardware_cpu_count () at 
 ../../../source/libcilkrts/runtime/os-unix.c:358
 358 {
 (gdb) return 1
 Make __cilkrts_hardware_cpu_count return now? (y or n) y
 #0  cilkg_get_user_settable_values () at 
 ../../../source/libcilkrts/runtime/global_state.cpp:385
 385 CILK_ASSERT(hardware_cpu_count  0);
 (gdb) c
 Continuing.
 ^C
 Program received signal SIGINT, Interrupt.
 f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
 9 while (!*steal_flag) 
 (gdb) info threads
   Id   Target Id Frame 
 * 1Thread 0x77fcd780 (LWP 30816) spawning_arg.ex f0 
 (steal_flag=steal_flag@entry=0x7fffd03c) at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
 (gdb) list
 4
 5   void f0(volatile int *steal_flag)
 6   { 
 7 int i = 0;
 8 /* Wait for steal_flag to be set */
 9 while (!*steal_flag) 
 10  ;
 11  }
 12
 13  int f1()
 (gdb) bt
 #0  f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
 #1  0x004009c8 in _cilk_spn_0 () at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
 #2  0x00400a4b in f1 () at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
 #3  0x00400d0e in _cilk_spn_1 () at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
 #4  0x00400d7a in f3 () at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
 #5  0x00400e33 in main () at 
 [...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:35
 
 No additional thread has been spawned by libcilkrts, and the one initial 
 thread is stuck in f0, without being able to make progress.  Should in f0's 
 while loop, some function be called to yield to libcilkrts scheduler, or 
 should libcilkrts have spawned an additional thread, or is the test case just 
 not valid Cilk Plus code?

Assuming the test cases are considered well-formed Cilk Plus code, I
understand there is then a hard requirement to run them with more than
one worker.  OK to fix as follows?

commit ee7138e451d1f3284d6fa0f61fe517c82db94060
Author: Thomas Schwinge tho...@codesourcery.com
Date:   Mon Sep 29 12:47:34 2014 +0200

Audit Cilk Plus tests for CILK_NWORKERS=1.

gcc/testsuite/
* c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
__cilkrts_set_param to set two workers.
* c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
* g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.
---
 gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c | 15 +++
 gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c  | 17 ++---
 gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc | 14 ++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git gcc/testsuite

RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Tannenbaum, Barry M]

In a nutshell, add the following code to main() before the call to f3():

int status = __cilkrts_set_param(nworkers, 2);
if (0 != status) {
// Failed to set the number of Cilk workers
return status;
}

Here's the details:

There are three sources of information the Cilk runtime uses to set the number 
of workers.

1) By default the Cilk runtime will query the operating system for the number 
of cores and create a worker for each core. Note that the operating system 
counts each hyperthread as a core.

2) You can set your own default by defining the CILK_NWORKERS environment 
variable.  For example:

export CILK_NWORKERS=2

3) You can override the default programmatically by calling 
__cilkrts_set_param() before the first spawning function is called. For 
example, place the following call in main():

__cilkrts_set_param(nworkers, 2);

A spawning function is a function that contains a _Cilk_spawn. The Cilk 
runtime will be initialized the first time a spawning function is executed.

For this test to run correctly you must set the number of workers to at least 
2, so the Cilk runtime will create at least one worker thread.

Please note that once the Cilk runtime has been initialized by the entry into 
the first spawning function, the number of workers cannot be changed (and the 
__cilkrts_set_param() call will return a non-zero value to indicate an error 
has occurred.) The first two values are evaluated lazily; the runtime will load 
its default values the first time one of the __cilkrts APIs defined in 
cilk_api.h is called [for example __cilkrts_get_nworkers()], or the first 
spawning function is entered. So for example, the following code will not do 
what you want:

if (1 == __cilkrts_get_nworkers())
setenv(CILK_NWORKERS=2);

The call to __cilkrts_get_nworkers() will cause the Cilk runtime to set its 
default. Setting the CILK_NWORKERS environment variable after the 
__cilkrts_get_nworkers() call will be too late.

It is possible to change the number of workers, but to do that you must return 
from all spawning functions and then call __cilkrts_end_cilk() to have the Cilk 
runtime shut down and then call __cilkrts_set_param(). When the Cilk runtime 
starts at the entry to the next spawning function, it will obey the new setting.

   - Barry

-Original Message-
From: Thomas Schwinge [mailto:tho...@codesourcery.com] 
Sent: Monday, September 29, 2014 6:54 AM
To: Tannenbaum, Barry M; Iyer, Balaji V; Zamyatin, Igor
Cc: gcc-patches@gcc.gnu.org
Subject: RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

Hi!

On Mon, 22 Sep 2014 19:21:33 +, Tannenbaum, Barry M 
barry.m.tannenb...@intel.com wrote:
 That's exactly correct.
 
 There are two ways to implement a work-stealing scheduler. We refer to 
 them as: [...]

Thanks for the explanation.

 Cilk implements Parent Stealing. Since you're running with 1 worker, there's 
 no other worker to steal the continuation. So steal_flag will never be set to 
 1 and you'll never break out of the loop.

Remains the question about how to address that in the testsuite:

 -Original Message-
 From: Thomas Schwinge [mailto:tho...@codesourcery.com]
 Sent: Monday, September 22, 2014 9:56 AM
 To: Iyer, Balaji V
 Cc: gcc-patches@gcc.gnu.org
 Subject: Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) 
 for C
 
 Hi!
 
 On Tue, 27 Aug 2013 21:30:49 +, Iyer, Balaji V 
 balaji.v.i...@intel.com wrote:
  --- /dev/null
  +++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
  @@ -0,0 +1,37 @@
  +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
  +/* { dg-options -fcilkplus } */
  +/* { dg-options -lcilkrts { target { i?86-*-* x86_64-*-* arm*-*-* 
  +} } } */
  +
  +void f0(volatile int *steal_flag)
  +{
  +  int i = 0;
  +  /* Wait for steal_flag to be set */
  +  while (!*steal_flag) 
  +;
  +}
  +
  +int f1()
  +{
  +
  +  volatile int steal_flag = 0;
  +  _Cilk_spawn f0(steal_flag);
  +  steal_flag = 1;  // Indicate stolen
  +  _Cilk_sync;
  +  return 0;
  +}
  +
  +void f2(int q)
  +{
  +  q = 5;
  +}
  +
  +void f3()
  +{
  +   _Cilk_spawn f2(f1());
  +}
  +
  +int main()
  +{
  +  f3();
  +  return 0;
  +}
 
 Is this really well-formed Cilk Plus code?  Running with CILK_NWORKERS=1, or 
 -- the equivalent -- in a system with just one CPU (as per 
 libcilkrts/runtime/os-unix.c:__cilkrts_hardware_cpu_count returning 1), I see 
 this test busy-loop as follows:
 
 Breakpoint 1, __cilkrts_hardware_cpu_count () at 
 ../../../source/libcilkrts/runtime/os-unix.c:358
 358 {
 (gdb) return 1
 Make __cilkrts_hardware_cpu_count return now? (y or n) y
 #0  cilkg_get_user_settable_values () at 
 ../../../source/libcilkrts/runtime/global_state.cpp:385
 385 CILK_ASSERT(hardware_cpu_count  0);
 (gdb) c
 Continuing.
 ^C
 Program received signal SIGINT, Interrupt.
 f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
 [...]/source

RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Thomas Schwinge]

Hi!

On Mon, 29 Sep 2014 13:58:31 +, Tannenbaum, Barry M 
barry.m.tannenb...@intel.com wrote:
 In a nutshell, add the following code to main() before the call to f3():
 
 int status = __cilkrts_set_param(nworkers, 2);
 if (0 != status) {
 // Failed to set the number of Cilk workers
 return status;
 }

Yeah, that's what I had proposed with the patch at the end of my previous
email,
http://news.gmane.org/find-root.php?message_id=%3C8761g6g0je.fsf%40kepler.schwinge.homeip.net%3E.
I'm sorry if I didn't make it obvious that more text and the patch were
following after the full-quote of the original issue description.

 Here's the details: [...]

Thanks again for your helpful comments; that's appreciated.

Here's again my proposed patch.  Note, that the include paths in GCC
compiler testing (gcc/testsuite/) are not set up to pick up the
cilk/cilk_api.h include file, so I've manually added a propotype for
the __cilkrts_set_param function to the three files.  I can change that,
if requested.

commit ee7138e451d1f3284d6fa0f61fe517c82db94060
Author: Thomas Schwinge tho...@codesourcery.com
Date:   Mon Sep 29 12:47:34 2014 +0200

Audit Cilk Plus tests for CILK_NWORKERS=1.

gcc/testsuite/
* c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
__cilkrts_set_param to set two workers.
* c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
* g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.
---
 gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c | 15 +++
 gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c  | 17 ++---
 gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc | 14 ++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
index 95e6cab..138b82c 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
@@ -2,6 +2,17 @@
 /* { dg-options -fcilkplus } */
 /* { dg-additional-options -lcilkrts { target { i?86-*-* x86_64-*-* } } } */
 
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
+
 void f0(volatile int *steal_flag)
 { 
   int i = 0;
@@ -32,6 +43,10 @@ void f3()
 
 int main()
 {
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   f3();
   return 0;
 }
diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
index 6e28765..6b41c7f 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
@@ -2,8 +2,16 @@
 /* { dg-options -fcilkplus } */
 /* { dg-additional-options -lcilkrts { target { i?86-*-* x86_64-*-* } } } */
 
-// #include cilk/cilk_api.h
-extern void __cilkrts_set_param (char *, char *);
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void foo(volatile int *);
 
@@ -11,7 +19,10 @@ void main2(void);
 
 int main(void)
 {
- //  __cilkrts_set_param ((char *)nworkers, (char *)2);
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   main2();
   return 0;
 }
diff --git gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc 
gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
index 0633d19..09ddf8b 100644
--- gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
+++ gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
@@ -10,6 +10,16 @@
 #endif
 #include cstdlib
 
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void func(int volatile* steal_me) 
 {
@@ -59,6 +69,10 @@ void my_test()
 
 int main() 
 {
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   my_test();
 #if HAVE_IO
   printf(PASSED\n);


Grüße,
 Thomas


pgpnNzunM87ZE.pgp
Description: PGP signature

RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Tannenbaum, Barry M]

Looks good to me. I apologize for spamming you with the extra details. I wasn't 
clear that this was a patch instead of a bug report.

I believe that Igor is the one who controls the GCC submission for Cilk Plus.

  - Barry

-Original Message-
From: Thomas Schwinge [mailto:tho...@codesourcery.com] 
Sent: Monday, September 29, 2014 10:27 AM
To: Tannenbaum, Barry M; Iyer, Balaji V; Zamyatin, Igor
Cc: gcc-patches@gcc.gnu.org
Subject: RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

Hi!

On Mon, 29 Sep 2014 13:58:31 +, Tannenbaum, Barry M 
barry.m.tannenb...@intel.com wrote:
 In a nutshell, add the following code to main() before the call to f3():
 
 int status = __cilkrts_set_param(nworkers, 2);
 if (0 != status) {
 // Failed to set the number of Cilk workers
 return status;
 }

Yeah, that's what I had proposed with the patch at the end of my previous 
email, 
http://news.gmane.org/find-root.php?message_id=%3C8761g6g0je.fsf%40kepler.schwinge.homeip.net%3E.
I'm sorry if I didn't make it obvious that more text and the patch were 
following after the full-quote of the original issue description.

 Here's the details: [...]

Thanks again for your helpful comments; that's appreciated.

Here's again my proposed patch.  Note, that the include paths in GCC compiler 
testing (gcc/testsuite/) are not set up to pick up the cilk/cilk_api.h 
include file, so I've manually added a propotype for the __cilkrts_set_param 
function to the three files.  I can change that, if requested.

commit ee7138e451d1f3284d6fa0f61fe517c82db94060
Author: Thomas Schwinge tho...@codesourcery.com
Date:   Mon Sep 29 12:47:34 2014 +0200

Audit Cilk Plus tests for CILK_NWORKERS=1.

gcc/testsuite/
* c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
__cilkrts_set_param to set two workers.
* c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
* g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.
---
 gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c | 15 +++  
gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c  | 17 ++---
 gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc | 14 ++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
index 95e6cab..138b82c 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
@@ -2,6 +2,17 @@
 /* { dg-options -fcilkplus } */
 /* { dg-additional-options -lcilkrts { target { i?86-*-* x86_64-*-* } } } */
 
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
+
 void f0(volatile int *steal_flag)
 { 
   int i = 0;
@@ -32,6 +43,10 @@ void f3()
 
 int main()
 {
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   f3();
   return 0;
 }
diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c 
gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
index 6e28765..6b41c7f 100644
--- gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
@@ -2,8 +2,16 @@
 /* { dg-options -fcilkplus } */
 /* { dg-additional-options -lcilkrts { target { i?86-*-* x86_64-*-* } } } */
 
-// #include cilk/cilk_api.h
-extern void __cilkrts_set_param (char *, char *);
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void foo(volatile int *);
 
@@ -11,7 +19,10 @@ void main2(void);
 
 int main(void)
 {
- //  __cilkrts_set_param ((char *)nworkers, (char *)2);
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   main2();
   return 0;
 }
diff --git gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc 
gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
index 0633d19..09ddf8b 100644
--- gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
+++ gcc/testsuite/g++.dg/cilk-plus/CK/catch_exc.cc
@@ -10,6 +10,16 @@
 #endif
 #include cstdlib
 
+#ifdef __cplusplus
+extern C {
+#endif
+
+extern int __cilkrts_set_param (const char *, const char *);
+
+#ifdef __cplusplus
+}
+#endif
+
 
 void func(int volatile* steal_me)
 {
@@ -59,6 +69,10 @@ void my_test()
 
 int main()
 {
+  /* Ensure more than one worker.  */
+  if (__cilkrts_set_param(nworkers, 2) != 0)
+__builtin_abort();
+
   my_test();
 #if HAVE_IO
   printf(PASSED\n);


Grüße,
 Thomas

Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Jeff Law]

On 09/29/14 08:26, Thomas Schwinge wrote:

Hi!

On Mon, 29 Sep 2014 13:58:31 +, Tannenbaum, Barry M 
barry.m.tannenb...@intel.com wrote:

In a nutshell, add the following code to main() before the call to f3():

 int status = __cilkrts_set_param(nworkers, 2);
 if (0 != status) {
 // Failed to set the number of Cilk workers
 return status;
 }


Yeah, that's what I had proposed with the patch at the end of my previous
email,
http://news.gmane.org/find-root.php?message_id=%3C8761g6g0je.fsf%40kepler.schwinge.homeip.net%3E.
I'm sorry if I didn't make it obvious that more text and the patch were
following after the full-quote of the original issue description.


Here's the details: [...]


Thanks again for your helpful comments; that's appreciated.

Here's again my proposed patch.  Note, that the include paths in GCC
compiler testing (gcc/testsuite/) are not set up to pick up the
cilk/cilk_api.h include file, so I've manually added a propotype for
the __cilkrts_set_param function to the three files.  I can change that,
if requested.

commit ee7138e451d1f3284d6fa0f61fe517c82db94060
Author: Thomas Schwinge tho...@codesourcery.com
Date:   Mon Sep 29 12:47:34 2014 +0200

 Audit Cilk Plus tests for CILK_NWORKERS=1.

gcc/testsuite/
* c-c++-common/cilk-plus/CK/spawning_arg.c (main): Call
__cilkrts_set_param to set two workers.
* c-c++-common/cilk-plus/CK/steal_check.c (main): Likewise.
* g++.dg/cilk-plus/CK/catch_exc.cc (main): Likewise.

OK.
Jeff

Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Thomas Schwinge]

Hi!

On Tue, 27 Aug 2013 21:30:49 +, Iyer, Balaji V balaji.v.i...@intel.com 
wrote:
 --- /dev/null
 +++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
 @@ -0,0 +1,37 @@
 +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
 +/* { dg-options -fcilkplus } */
 +/* { dg-options -lcilkrts { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
 +
 +void f0(volatile int *steal_flag)
 +{ 
 +  int i = 0;
 +  /* Wait for steal_flag to be set */
 +  while (!*steal_flag) 
 +;
 +}
 +
 +int f1()
 +{
 +
 +  volatile int steal_flag = 0;
 +  _Cilk_spawn f0(steal_flag);
 +  steal_flag = 1;  // Indicate stolen
 +  _Cilk_sync; 
 +  return 0;
 +}
 +
 +void f2(int q)
 +{
 +  q = 5;
 +}
 +
 +void f3()
 +{
 +   _Cilk_spawn f2(f1());
 +}
 +
 +int main()
 +{
 +  f3();
 +  return 0;
 +}

Is this really well-formed Cilk Plus code?  Running with CILK_NWORKERS=1,
or -- the equivalent -- in a system with just one CPU (as per
libcilkrts/runtime/os-unix.c:__cilkrts_hardware_cpu_count returning 1), I
see this test busy-loop as follows:

Breakpoint 1, __cilkrts_hardware_cpu_count () at 
../../../source/libcilkrts/runtime/os-unix.c:358
358 {
(gdb) return 1
Make __cilkrts_hardware_cpu_count return now? (y or n) y
#0  cilkg_get_user_settable_values () at 
../../../source/libcilkrts/runtime/global_state.cpp:385
385 CILK_ASSERT(hardware_cpu_count  0);
(gdb) c
Continuing.
^C
Program received signal SIGINT, Interrupt.
f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
9 while (!*steal_flag) 
(gdb) info threads
  Id   Target Id Frame 
* 1Thread 0x77fcd780 (LWP 30816) spawning_arg.ex f0 
(steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
(gdb) list
4
5   void f0(volatile int *steal_flag)
6   { 
7 int i = 0;
8 /* Wait for steal_flag to be set */
9 while (!*steal_flag) 
10  ;
11  }
12
13  int f1()
(gdb) bt
#0  f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
#1  0x004009c8 in _cilk_spn_0 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
#2  0x00400a4b in f1 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
#3  0x00400d0e in _cilk_spn_1 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
#4  0x00400d7a in f3 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
#5  0x00400e33 in main () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:35

No additional thread has been spawned by libcilkrts, and the one initial
thread is stuck in f0, without being able to make progress.  Should in
f0's while loop, some function be called to yield to libcilkrts
scheduler, or should libcilkrts have spawned an additional thread, or is
the test case just not valid Cilk Plus code?


Grüße,
 Thomas


pgpG_DU89ebEh.pgp
Description: PGP signature

RE: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Tannenbaum, Barry M]

That's exactly correct.

There are two ways to implement a work-stealing scheduler. We refer to them as:

   - Child Stealing - This is what TBB implements. The spawned call is bundled 
up with all the parameters and pushed onto a queue. The worker will continue 
executing the code after the spawned function until a sync, at which point it 
will go back and execute any spawned functions that haven't already been stolen.

   - Parent Stealing - When a _Cilk_spawn is executed, an annotation is made on 
the worker's deque indicating that the continuation (the code following the 
_Cilk_spawn call) is available for stealing, and the worker starts executing 
the spawned function. When the spawned function returns, a check is made to 
determine whether the parent has been stolen. If it hasn't, then the spawned 
function returns normally. If it has, then execution jumps to the sync.

Cilk implements Parent Stealing. Since you're running with 1 worker, there's no 
other worker to steal the continuation. So steal_flag will never be set to 1 
and you'll never break out of the loop.

One of the advantages of Child Stealing is that it's very light weight. It 
requires a jmpbuf (in the __cilkrts_stack_frame allocated in the spawning 
function) and a char * in the worker's deque.

   - Barry (Intel Cilk Plus Development)


-Original Message-
From: Thomas Schwinge [mailto:tho...@codesourcery.com] 
Sent: Monday, September 22, 2014 9:56 AM
To: Iyer, Balaji V
Cc: gcc-patches@gcc.gnu.org
Subject: Re: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

Hi!

On Tue, 27 Aug 2013 21:30:49 +, Iyer, Balaji V balaji.v.i...@intel.com 
wrote:
 --- /dev/null
 +++ gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
 @@ -0,0 +1,37 @@
 +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
 +/* { dg-options -fcilkplus } */
 +/* { dg-options -lcilkrts { target { i?86-*-* x86_64-*-* arm*-*-* } 
 +} } */
 +
 +void f0(volatile int *steal_flag)
 +{
 +  int i = 0;
 +  /* Wait for steal_flag to be set */
 +  while (!*steal_flag) 
 +;
 +}
 +
 +int f1()
 +{
 +
 +  volatile int steal_flag = 0;
 +  _Cilk_spawn f0(steal_flag);
 +  steal_flag = 1;  // Indicate stolen
 +  _Cilk_sync;
 +  return 0;
 +}
 +
 +void f2(int q)
 +{
 +  q = 5;
 +}
 +
 +void f3()
 +{
 +   _Cilk_spawn f2(f1());
 +}
 +
 +int main()
 +{
 +  f3();
 +  return 0;
 +}

Is this really well-formed Cilk Plus code?  Running with CILK_NWORKERS=1, or -- 
the equivalent -- in a system with just one CPU (as per 
libcilkrts/runtime/os-unix.c:__cilkrts_hardware_cpu_count returning 1), I see 
this test busy-loop as follows:

Breakpoint 1, __cilkrts_hardware_cpu_count () at 
../../../source/libcilkrts/runtime/os-unix.c:358
358 {
(gdb) return 1
Make __cilkrts_hardware_cpu_count return now? (y or n) y
#0  cilkg_get_user_settable_values () at 
../../../source/libcilkrts/runtime/global_state.cpp:385
385 CILK_ASSERT(hardware_cpu_count  0);
(gdb) c
Continuing.
^C
Program received signal SIGINT, Interrupt.
f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
9 while (!*steal_flag) 
(gdb) info threads
  Id   Target Id Frame 
* 1Thread 0x77fcd780 (LWP 30816) spawning_arg.ex f0 
(steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
(gdb) list
4
5   void f0(volatile int *steal_flag)
6   { 
7 int i = 0;
8 /* Wait for steal_flag to be set */
9 while (!*steal_flag) 
10  ;
11  }
12
13  int f1()
(gdb) bt
#0  f0 (steal_flag=steal_flag@entry=0x7fffd03c) at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:9
#1  0x004009c8 in _cilk_spn_0 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
#2  0x00400a4b in f1 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:17
#3  0x00400d0e in _cilk_spn_1 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
#4  0x00400d7a in f3 () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:30
#5  0x00400e33 in main () at 
[...]/source/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c:35

No additional thread has been spawned by libcilkrts, and the one initial thread 
is stuck in f0, without being able to make progress.  Should in f0's while 
loop, some function be called to yield to libcilkrts scheduler, or should 
libcilkrts have spawned an additional thread, or is the test case just not 
valid Cilk Plus code?


Grüße,
 Thomas

Re: _Cilk_spawn and _Cilk_sync for C++

[Andreas Schwab]

Iyer, Balaji V balaji.v.i...@intel.com writes:

 diff --git a/gcc/testsuite/g++.dg/cilk-plus/cilk-plus.exp 
 b/gcc/testsuite/g++.dg/cilk-plus/cilk-plus.exp
 index 707d17e..36c8111 100644
 --- a/gcc/testsuite/g++.dg/cilk-plus/cilk-plus.exp
 +++ b/gcc/testsuite/g++.dg/cilk-plus/cilk-plus.exp
 @@ -22,6 +22,14 @@ if { ![check_effective_target_cilkplus] } {
  return;
  }
  
 +verbose $tool $libdir 1
 +set library_var [get_multilibs]
 +# Pointing the ld_library_path to the Cilk Runtime library binaries.
 +set ld_library_path $[get_multilibs]/libcilkrts/.libs
 +
 +set ALWAYS_CFLAGS 
 +lappend ALWAYS_CFLAGS -L${library_var}/libcilkrts/.libs

This is broken and useless.  Remove that.

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
And now for something completely different.

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 12/10/2013 06:03 PM, Iyer, Balaji V wrote:

Fixed patch and ChangeLog entries are attached. Is it Ok to install?


OK.

Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 12/05/2013 11:38 PM, Iyer, Balaji V wrote:

used the init_p value  that comes out of stabilize_expr


I guess you didn't look at the patch I sent you...

Since you've fixed extract_free_variables, you don't need 
call_to_lambda_fn_p at all, or to call stabilize_expr.


Why do you need to move add_variable_type, cilk_block_type and 
wrapper_data out of c-family/cilk.c?  You don't need to have the 
definition of wrapper_data available in order to cast a void* to a 
wrapper_data*.


Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 12/04/2013 02:45 PM, Jason Merrill wrote:

+   error_at (input_location, _Cilk_sync cannot be used without
enabling
+ Cilk Plus);
+  cp_lexer_consume_token (parser-lexer);
+  if (parser-in_statement  IN_CILK_SPAWN)
+   parser-in_statement = parser-in_statement  ~IN_CILK_SPAWN;


Why are you messing with in_statement in the cilk_spawn code?


This needed to catch cases like this:

_Cilk_spawn _Cilk_spawn foo ()


Oops, I meant to say in the cilk_sync code.  Why does finding a
_Cilk_sync end the _Cilk_spawn context?


Ping, I think this question got lost in the discussion of the copy_body 
function.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Thursday, December 5, 2013 4:00 PM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 12/04/2013 02:45 PM, Jason Merrill wrote:
  +   error_at (input_location, _Cilk_sync cannot be used
  + without
  enabling
  + Cilk Plus);
  +  cp_lexer_consume_token (parser-lexer);
  +  if (parser-in_statement  IN_CILK_SPAWN)
  +   parser-in_statement = parser-in_statement 
  + ~IN_CILK_SPAWN;
 
  Why are you messing with in_statement in the cilk_spawn code?
 
  This needed to catch cases like this:
 
  _Cilk_spawn _Cilk_spawn foo ()
 
  Oops, I meant to say in the cilk_sync code.  Why does finding a
  _Cilk_sync end the _Cilk_spawn context?
 
 Ping, I think this question got lost in the discussion of the copy_body
 function.
 

It doesn't. I just have an if statement to clear out IN_CILK_SPAWN if it is was 
still enabled (more like a safety measure). I have removed it.

 Jason
 

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 12/03/2013 07:08 PM, Iyer, Balaji V wrote:

In install_body_with_frame_cleanup for C++, I am using trees such as 
TRY_CATCH_EXPR and am using a function from the cp/except.c. I didn't know how 
to bring them to c-family.


I had in mind that the declaration would be in c-common.h, but each 
front end would have a different definition in the front end directory, 
kind of like how all front ends need to define convert.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Wednesday, December 4, 2013 5:39 PM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 12/03/2013 07:08 PM, Iyer, Balaji V wrote:
  In install_body_with_frame_cleanup for C++, I am using trees such as
 TRY_CATCH_EXPR and am using a function from the cp/except.c. I didn't
 know how to bring them to c-family.
 
 I had in mind that the declaration would be in c-common.h, but each front
 end would have a different definition in the front end directory, kind of like
 how all front ends need to define convert.

I didn't know it was an OK thing to do. Okie dokey. I will work on this and the 
previous email comment you send me and will send out a patch tomorrow.

Thanks,

Balaji V. Iyer.

 
 Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 12/04/2013 05:42 PM, Iyer, Balaji V wrote:

I had in mind that the declaration would be in c-common.h, but each front
end would have a different definition in the front end directory, kind of like
how all front ends need to define convert.


I didn't know it was an OK thing to do.


I think it's OK for c-common interfaces, but not for back end interfaces.

Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 
  case CILK_SPAWN_STMT:
gcc_assert
  (fn_contains_cilk_spawn_p (cfun)
lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
if (!seen_error ())
  {
ret = (enum gimplify_status)
  lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
   post_p);
break;
  }
/* If errors are seen, then just process it as a CALL_EXPR.
  */
 
 Please remove these langhooks and instead add handling of
 CILK_SPAWN_STMT to c_gimplify_expr and cp_gimplify_expr.
 
Hi Jason,
I really cannot do this because if the spawned function returns a 
value, the whole expression must be pushed into the spawn helper. Thus, this 
lang_hooks function is called in the gimplify_modify_expr and a couple other 
places.

E.g.  In:

x = _Cilk_spawn func ()

The spawn helper should contain:

x = func ();

Thanks,

Balaji V. Iyer.

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/28/2013 11:40 AM, Iyer, Balaji V wrote:

Consider the following test case. I took this from the lambda_spawns.cc line 
#203.

as you can tell, it is clobbering the lambda closure at the end of the lambda 
calling and then it is catching value of A from main2 as it is supposed to.


Yep, your patch gives a fine result for this testcase.


What am I misunderstanding?


It just gets there for the wrong reason: remapping exactly nothing in 
the closure object happens to give the desired semantics, treating the 
temporaries in the CONSTRUCTOR as local to the spawned function and 
referring to variables from the spawning context via the nested function 
static chain.  But presumably you have all that remapping machinery 
there because doing nothing doesn't always give the desired result.  Right?


When you add CONSTRUCTOR handling to extract_free_variables, you get the 
crash in gimplify_var_or_parm_decl because you don't specifically handle 
VEC_INIT_EXPR, which needs to be handled a lot like TARGET_EXPR, and end 
up trying to pass the address of its temporary object into the spawned 
function, which doesn't work because, like a TARGET_EXPR, the temporary 
doesn't exist outside of the VEC_INIT_EXPR.  This doesn't mean that 
handling CONSTRUCTOR is wrong; leaving it out means that you aren't 
going to handle aggregate temporaries properly either.


I think it might be better for gimplify_cilk_spawn to gimplify the call 
expression first, and then do your transformation on the gimple, so you 
don't have to worry about language-specific magic.


Now, after all that I must admit that cilk_spawn could only ever see 
VEC_INIT_EXPR in the context of a lambda closure initialization, and the 
default behavior should always be correct for a lambda closure 
initialization, so I guess I'm willing to allow the magic lambda 
handling with a comment about it being a workaround.



+is_lambda_fn_p (tree call_exp)
+{
+  if (TREE_CODE (call_exp) != CALL_EXPR)
+return false;
+  tree call_fn = CALL_EXPR_FN (call_exp);
+  if (TREE_CODE (call_fn) == ADDR_EXPR)
+call_fn = TREE_OPERAND (call_fn, 0);


Use get_callee_fndecl to get the FUNCTION_DECL.  And change the name of 
the function, since it isn't testing whether the argument is itself a 
lambda function; perhaps call_to_lambda_fn_p?



case CILK_SPAWN_STMT:
  gcc_assert
(fn_contains_cilk_spawn_p (cfun)
  lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
  if (!seen_error ())
{
  ret = (enum gimplify_status)
lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
 post_p);
  break;
}
  /* If errors are seen, then just process it as a CALL_EXPR.  */


Please remove these langhooks and instead add handling of 
CILK_SPAWN_STMT to c_gimplify_expr and cp_gimplify_expr.



  lang_hooks.cilkplus.install_body_with_frame_cleanup (fndecl, stmt,
   (void *) wd);


And instead of this langhook, declare a function in c-common.h that is 
defined by all C family front ends.



+stabilize_expr (orig_body, pre_body);


Here you're pre-evaluating the entire call, rather than just the lambda 
closure object, which means none of the arguments to the call will be 
remapped.  I think you want


CALL_EXPR_ARG (orig_body, 0)
  = stabilize_expr (CALL_EXPR_ARG (orig_body, 0), pre_body);
append_to_statement_list (orig_body, pre_body);

instead.


+  gcc_assert (TREE_CODE (catch_list) == STATEMENT_LIST);


You don't need this, append_to_statement_list handles the list not yet 
being a list fine.



+   /* We set this here so that finish_call_expr can set lambda to a var.
+  if it is not done so.  */


This comment is obsolete.


+   error_at (input_location, _Cilk_sync cannot be used without enabling 
+ Cilk Plus);
+  cp_lexer_consume_token (parser-lexer);
+  if (parser-in_statement  IN_CILK_SPAWN)
+   parser-in_statement = parser-in_statement  ~IN_CILK_SPAWN;


Why are you messing with in_statement in the cilk_spawn code?

Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/27/2013 11:05 PM, Iyer, Balaji V wrote:

Found the bug. I was not utilizing the stabilize_expr's output correctly.


Unfortunately, I think I was misleading you with talk of stabilize; like 
you said, you want to evaluate the whole expression in the spawned 
function rather than in the caller, so that any temporaries (including 
the lambda closure) live until the _Cilk_sync.  Using stabilize_expr 
this way (the way I was suggesting) forces the lambda closure to be 
evaluated in the caller, and then destroyed at the end of the enclosing 
statement, which is likely to erase any data that the spawned function 
needs to do its work, if anything captured by copy has a destructor.


As I said in my last mail, I think the right fix is to make sure that A 
gets remapped properly during copy_body so that its use in the 
initializer for the closure doesn't confuse later passes.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Thursday, November 28, 2013 9:11 AM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/27/2013 11:05 PM, Iyer, Balaji V wrote:
  Found the bug. I was not utilizing the stabilize_expr's output correctly.
 
 Unfortunately, I think I was misleading you with talk of stabilize; like you 
 said,
 you want to evaluate the whole expression in the spawned function rather
 than in the caller, so that any temporaries (including the lambda closure) 
 live
 until the _Cilk_sync.  Using stabilize_expr this way (the way I was 
 suggesting)
 forces the lambda closure to be evaluated in the caller, and then destroyed
 at the end of the enclosing statement, which is likely to erase any data that
 the spawned function needs to do its work, if anything captured by copy has
 a destructor.
 

 As I said in my last mail, I think the right fix is to make sure that A gets
 remapped properly during copy_body so that its use in the initializer for the
 closure doesn't confuse later passes.

Consider the following test case. I took this from the lambda_spawns.cc line 
#203.


  global_var = 0;
  _Cilk_spawn [=](int *Aa, int size){ foo1_c(A, size); }(B, 2);
  foo1 (A, 2);
  _Cilk_sync;
  if (global_var != 2)
return (++q);


... and here is its gimple output:

{
  struct  * D.2349;
  unsigned long D.2350;
  struct  * D.2351;
  struct  * D.2352;
  struct  * D.2353;
  struct  * D.2354;
  unsigned long D.2355;
  struct  * D.2356;
  struct  * D.2357;
  struct  * D.2358;
  struct  * D.2359;
  struct  * D.2360;
  struct __lambda0 D.2219;
  unsigned int D.2361;
  unsigned int D.2362;
  void * D.2363;
  void * D.2364;
  struct  * D.2365;
  struct  * D.2366;
  unsigned int D.2367;

  try
{
  try
{
  __cilkrts_enter_frame_fast_1 (D.2258);
  D.2349 = D.2258.worker;
  D.2350 = D.2349-pedigree.rank;
  D.2258.pedigree.rank = D.2350;
  D.2351 = D.2258.worker;
  D.2352 = D.2351-pedigree.parent;
  D.2258.pedigree.parent = D.2352;
  D.2353 = D.2258.call_parent;
  D.2354 = D.2258.worker;
  D.2355 = D.2354-pedigree.rank;
  D.2353-pedigree.rank = D.2355;
  D.2356 = D.2258.call_parent;
  D.2357 = D.2258.worker;
  D.2358 = D.2357-pedigree.parent;
  D.2356-pedigree.parent = D.2358;
  D.2359 = D.2258.worker;
  D.2359-pedigree.rank = 0;
  D.2360 = D.2258.worker;
  D.2360-pedigree.parent = D.2258.pedigree;
  __cilkrts_detach (D.2258);
  D.2219.__A = CHAIN.6-A;
  try
{
  main2(int)::lambda(int*, int)::operator() (D.2219, D.2255, 2);
}
 finally
{
  D.2219 = {CLOBBER};   
===
}
}
  catch
{
  catch (NULL)
{
  try
{
  D.2361 = D.2258.flags;
  D.2362 = D.2361 | 16;
  D.2258.flags = D.2362;
  D.2363 = __builtin_eh_pointer (0);
  D.2258.except_data = D.2363;
  D.2364 = __builtin_eh_pointer (0);
  __cxa_begin_catch (D.2364);
  __cxa_rethrow ();
}
  finally
{
  __cxa_end_catch ();
}
}
}
  finally

as you can tell, it is clobbering the lambda closure at the end of the lambda 
calling (in the finally expr, I marked with =  ) and then it is 
catching value of A from main2 as it is supposed to. 

What am I misunderstanding?


 
 Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Thursday, November 28, 2013 9:11 AM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/27/2013 11:05 PM, Iyer, Balaji V wrote:
  Found the bug. I was not utilizing the stabilize_expr's output correctly.
 
 Unfortunately, I think I was misleading you with talk of stabilize; like you 
 said,
 you want to evaluate the whole expression in the spawned function rather
 than in the caller, so that any temporaries (including the lambda closure) 
 live
 until the _Cilk_sync.  Using stabilize_expr this way (the way I was 
 suggesting)
 forces the lambda closure to be evaluated in the caller, and then destroyed
 at the end of the enclosing statement, which is likely to erase any data that
 the spawned function needs to do its work, if anything captured by copy has
 a destructor.
 
 As I said in my last mail, I think the right fix is to make sure that A gets
 remapped properly during copy_body so that its use in the initializer for the
 closure doesn't confuse later passes.

Ok. I think I cut and pasted the wrong part. I am very sorry about it.

Here is the original code:

  global_var = 0;
  _Cilk_spawn [=](int *Aa, int size){ foo1_c(A, size); }(B, 2);
  foo1 (A, 2);
  _Cilk_sync;
  if (global_var != 2)
return (++q);

Here is the gimple output with _Cilk_spawn and _Cilk_sync *DISABLED*

try
{
  A[0] = 5;
  A[1] = 3;
  B[0] = 5;
  B[1] = 3;
  main_size = argc + 1;
  q = 0;
  global_var = 0;
  D.2219.__A = A;
  try
{
  main2(int)::lambda(int*, int)::operator() (D.2219, B, 2);
}
  finally
{
  D.2219 = {CLOBBER};
}
  foo1 (A, 2);
  global_var.4 = global_var;
  if (global_var.4 != 2) goto D.2255; else goto D.2256;
  D.2255:
  q = q + 1;
  D.2257 = q;
  return D.2257;
  D.2256:
  D.2257 = q;
  return D.2257;
}
  finally
{
  A = {CLOBBER};
  B = {CLOBBER};
}

Here is the gimple output with _Cilk_spawn and _Cilk_synd enabled
try
{
  try
{
  __cilkrts_enter_frame_fast_1 (D.2258);
  D.2351 = D.2258.worker;
  D.2352 = D.2351-pedigree.rank;
  D.2258.pedigree.rank = D.2352;
  D.2353 = D.2258.worker;
  D.2354 = D.2353-pedigree.parent;
  D.2258.pedigree.parent = D.2354;
  D.2355 = D.2258.call_parent;
  D.2356 = D.2258.worker;
  D.2357 = D.2356-pedigree.rank;
  D.2355-pedigree.rank = D.2357;
  D.2358 = D.2258.call_parent;
  D.2359 = D.2258.worker;
  D.2360 = D.2359-pedigree.parent;
  D.2358-pedigree.parent = D.2360;
  D.2361 = D.2258.worker;
  D.2361-pedigree.rank = 0;
  D.2362 = D.2258.worker;
  D.2362-pedigree.parent = D.2258.pedigree;
  __cilkrts_detach (D.2258);
  B.5 = CHAIN.7-B;
  D.2219.__A = CHAIN.7-A;
  try
{
  main2(int)::lambda(int*, int)::operator() (D.2219, B.5, 2);
}
  finally
{
  D.2219 = {CLOBBER}; 
 CLOBBERING LINE
}
}
  catch
{
  catch (NULL)
{
  try
{
  D.2364 = D.2258.flags;
  D.2365 = D.2364 | 16;
  D.2258.flags = D.2365;
  D.2366 = __builtin_eh_pointer (0);
  D.2258.except_data = D.2366;
  D.2367 = __builtin_eh_pointer (0);
  __cxa_begin_catch (D.2367);
  __cxa_rethrow ();
}
  finally
{
  __cxa_end_catch ();
}
}
}
}
  finally
{
  D.2368 = D.2258.worker;
  D.2369 = D.2258.call_parent;
  D.2368-current_stack_frame = D.2369;
  __cilkrts_pop_frame (D.2258);
  D.2370 = D.2258.flags;
  if (D.2370 != 16777216) goto D.2371; else goto D.2372;
  D.2371:
  __cilkrts_leave_frame (D.2258);
  goto D.2373;
  D.2372:
  D.2373:
}
}

In this line (=) it is clobbering the lambda closure. 

Sorry again about the mistake.

By the way, I have only cut and pasted the part from the top-level try 
expression to just show the relevant part. If you want, I can show you the rest.

 
 Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/25/2013 10:50 AM, Iyer, Balaji V wrote:

I have fixed this issue.  My function to map the variable's context from the 
spawner to the spawn helper function was going into the lambda function. I made 
it stop by adding a language specific copy_tree_body (basically stop going into 
the lambda function's body for C++ and for the rest of the times just use 
copy_tree_body_r, no code duplicating  is done between the two) that and it 
works fine now.


I doubt it was walking from the enclosing function into the body of the 
lambda function.  Looking at the patch, it seems that what you're 
avoiding is walking into the closure object itself, and adding an entire 
new langhook seems like overkill for that.


I think a better approach would be to add a cp_build_cilk_spawn that 
uses stabilize_call to pre-evaluate the arguments of the call.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Wednesday, November 27, 2013 12:43 PM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/25/2013 10:50 AM, Iyer, Balaji V wrote:
  I have fixed this issue.  My function to map the variable's context from the
 spawner to the spawn helper function was going into the lambda function. I
 made it stop by adding a language specific copy_tree_body (basically stop
 going into the lambda function's body for C++ and for the rest of the times
 just use copy_tree_body_r, no code duplicating  is done between the two)
 that and it works fine now.
 
 I doubt it was walking from the enclosing function into the body of the
 lambda function.  Looking at the patch, it seems that what you're avoiding is
 walking into the closure object itself, and adding an entire new langhook
 seems like overkill for that.
 
 I think a better approach would be to add a cp_build_cilk_spawn that uses
 stabilize_call to pre-evaluate the arguments of the call.

I really can't pre-evaluate the calls before I move into the nested function 
because all those parts must be in the nested function.

Adding another hook seem to be straightforward for me. One advantage I can 
think of having a separate node for that when new features get added in, we 
have a place to separately evaluate them and copy them as necessary. If it is 
not too much of a hazzle, I would like to keep them. I don't add anything more 
in the structure, just 1 more for _Cilk_for.

Or, another thing I could do is to pass a function pointer and pass the 
copy_tree_body function into it. Is that acceptable by you?

 
 Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/27/2013 01:25 PM, Iyer, Balaji V wrote:

I think a better approach would be to add a cp_build_cilk_spawn that uses
stabilize_call to pre-evaluate the arguments of the call.


I really can't pre-evaluate the calls before I move into the nested function 
because all those parts must be in the nested function.


OK, then use stabilize_expr to pre-evaluate just the function, to get 
the effect of your earlier cilk_create_lambda_fn_tmp_var without the 
undesirable lifetime effects.



Adding another hook seem to be straightforward for me.


It's straightforward but undesirable; we want to minimize the number of 
hooks, and this one is unnecessary.


Jason

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/27/2013 05:59 PM, Iyer, Balaji V wrote:

Well, if I use copy_tree_body_r for C and C++, in lambda functions, it asserts 
in varasm.c. The main reason I see that, the copy_tree_body_r walks into the 
closure and then maps the variables from the lambda function from the spawner 
to the helper function.


It walks into the initializer for the closure object, yes.  And gets 
confused by the initializer in the


  _Cilk_spawn [=](int *Aa, int size){ foo1_c(A, size); }(B, 2);

case.  The initializer for the closure is a CONSTRUCTOR which contains a 
reference to A, and that is running into trouble because 
extract_free_variables doesn't walk into CONSTRUCTORs.  You probably 
want to switch a lot of your tree walking functions to use walk_tree 
directly rather than try to keep up with all the different kinds of 
expressions yourself.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Wednesday, November 27, 2013 8:24 PM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/27/2013 05:59 PM, Iyer, Balaji V wrote:
  Well, if I use copy_tree_body_r for C and C++, in lambda functions, it
 asserts in varasm.c. The main reason I see that, the copy_tree_body_r walks
 into the closure and then maps the variables from the lambda function from
 the spawner to the helper function.
 
 It walks into the initializer for the closure object, yes.  And gets confused 
 by
 the initializer in the
 
_Cilk_spawn [=](int *Aa, int size){ foo1_c(A, size); }(B, 2);
 
 case.  The initializer for the closure is a CONSTRUCTOR which contains a
 reference to A, and that is running into trouble because
 extract_free_variables doesn't walk into CONSTRUCTORs.  You probably
 want to switch a lot of your tree walking functions to use walk_tree directly
 rather than try to keep up with all the different kinds of expressions 
 yourself.

Found the bug. I was not utilizing the stabilize_expr's output correctly. Here 
is the fixed patch. I have removed the function pointer and the lang_hooks 
addition. It now just uses copy_tree_body_r for C and C++. It passes all the 
tests for C and C++ on my x86_64 box.

Is this Ok for trunk?

Here are the ChangeLog entries:
gcc/c-family/ChangeLog
2013-11-27  Balaji V. Iyer  balaji.v.i...@intel.com

* cilk.c (cilk_outline): Made this function non-static.
* c-common.h (cilk_outline): New prototype.
(c_cilk_install_body_with_frame_cleanup): Added a new parameter.

gcc/ChangeLog
2013-11-27  Balaji V. Iyer  balaji.v.i...@intel.com

* langhooks.h
(lang_hooks_for_cilkplus::install_body_with_frame_cleanup): Added new
parameter.

gcc/cp/ChangeLog
2013-11-27  Balaji V. Iyer  balaji.v.i...@intel.com

* cp-tree.h (cilk_valid_spawn): New prototype.
(gimplify_cilk_spawn): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(cilk_create_lambda_fn_tmp_var): Likewise.
(create_try_catch_expr): Likewise.
* decl.c (finish_function): Insert Cilk function-calls when a
_Cilk_spawn is used in a function.
* parser.c (cp_parser_postfix_expression): Added RID_CILK_SPAWN and
RID_CILK_SYNC cases.
* cp-cilkplus.c (set_cilk_except_flag): New function.
(set_cilk_except_data): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(is_lambda_function): Likewise.
* except.c (create_try_catch_expr): Likewise.
* parser.h (IN_CILK_SPAWN): New #define.
* cp-objcp-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): Likewise.
* cp-objcp-common.h (LANG_HOOKS_CILKPLUS_CILK_SPEC_COPY_BODY):
Likewise.
(LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
* pt.c (tsubst_expr): Added CILK_SPAWN_STMT and CILK_SYNC_STMT cases.
* semantics.c (potential_constant_expression_1): Likewise.
* typeck.c (cp_build_compound_expr): Reject a spawned function in a
compound expression.
(check_return_expr): Reject a spawned function in a return expression.

gcc/testsuite/ChangeLog
2013-11-27  Balaji V. Iyer  balaji.v.i...@intel.com

* g++.dg/cilk-plus/CK/catch_exc.cc: New test case.
* g++.dg/cilk-plus/CK/const_spawn.cc: Likewise.
* g++.dg/cilk-plus/CK/fib-opr-overload.cc: Likewise.
* g++.dg/cilk-plus/CK/fib-tplt.cc: Likewise.
* g++.dg/cilk-plus/CK/lambda_spawns.cc: Likewise.
* g++.dg/cilk-plus/CK/lambda_spawns_tplt.cc: Likewise.
* g++.dg/cilk-plus/cilk-plus.exp: Added support to run Cilk Keywords
test stored in c-c++-common.  Also, added the Cilk runtime's library
to the ld_library_path.

Thanks,

Balaji V. Iyer.
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 664e928..f46008f
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1379,7 +1379,7 @@ extern vec tree, va_gc *fix_sec_implicit_args
 extern tree insert_cilk_frame (tree);
 extern void cilk_init_builtins (void);
 extern int gimplify_cilk_spawn (tree *, gimple_seq *, gimple_seq *);
-extern void c_cilk_install_body_w_frame_cleanup (tree, tree);
+extern void c_cilk_install_body_w_frame_cleanup (tree, tree, void *);
 extern bool cilk_detect_spawn_and_unwrap (tree *);
 extern bool cilk_set_spawn_marker (location_t, tree);
 extern tree build_cilk_sync (void);
@@ -1387,5 +1387,5 @@ extern tree build_cilk_spawn (location_t, tree);
 extern tree make_cilk_frame (tree);
 extern tree create_cilk_function_exit (tree, bool, bool);
 extern tree cilk_install_body_pedigree_operations (tree);
-
+extern void cilk_outline (tree, tree *, struct wrapper_data *);
 #endif /* ! GCC_C_COMMON_H */
diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
index c85b5f2

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

Hi Jason,
Please see my responses below

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Friday, November 22, 2013 10:51 AM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/21/2013 05:40 PM, Iyer, Balaji V wrote:
  +/* Returns a TRY_CATCH_EXPR that will encapsulate BODY, EXCEPT_DATA
 and
  +   EXCEPT_FLAG.  */
  +
  +tree
  +create_cilk_try_catch (tree except_flag, tree except_data, tree body)
  +{
  +  tree catch_list = alloc_stmt_list ();
  +  append_to_statement_list (except_flag, catch_list);
  +  append_to_statement_list (except_data, catch_list);
  +  append_to_statement_list (do_begin_catch (), catch_list);
  +  append_to_statement_list (build_throw (NULL_TREE), catch_list);
  +  tree catch_tf_expr = build_stmt (EXPR_LOCATION (body),
 TRY_FINALLY_EXPR,
  +  catch_list, do_end_catch (NULL_TREE));
  +  catch_list = build2 (CATCH_EXPR, void_type_node, NULL_TREE,
  +  catch_tf_expr);
  +  tree try_catch_expr = build_stmt (EXPR_LOCATION (body),
 TRY_CATCH_EXPR,
  +   body, catch_list);
  +  return try_catch_expr;
  +}
 
 I had in mind something less cilk-specific: a function that takes two tree
 operands, one for the body and one for the throwing path.
 Basically, make catch_list a parameter and move the first two appends back
 into the calling function.
 

This is fixed as you suggested.


  The reason is that, when you have something like this:
 
  _Cilk_spawn [=]  { body } ();
 
  I need to capture the function call (which in this case is the whole 
  function)
 and throw it into a nested function.  The nested function implementation is
 shared with C. If the function is stored in a variable then I can just send 
 that
 out to the nested function. I have added another constraint to make sure the
 function is a spawning function, this way we can reduce more cases were
 they are stored to a variable. The reason why I added this check in
 finish_call_expr is that it seemed to be most straight-forward for me and
 only place where I could do with least disruption (code-changes).
 
 It looks like you're transforming any
 
 [...] {...} (...);
 
 into
 
 auto lambda = [...]{...};
 lambda(...);
 
 which has significantly different semantics, particularly in terms of the
 lifetime of the lambda object.  In some of the Cilk online documentation, I
 see:
 
  When spawning named lambda functions, be careful that the lifespan of
 the lambda extends at least until the next sync, or else the destructor for 
 the
 lambda will race with the spawned call. For example:
  double i = g();
  if (some condition) {
 // named lambda with value capture of i
 auto f = [=i]() { double d = sin(i); f(d); };
 cilk_spawn f();
  } // Ouch! destructor for f is in parallel with spawned call.
 
 This would seem to apply even more to unnamed lambda functions, since
 normally they would be destroyed at the end of the full-expression, which
 must always be before the sync.  Does the Intel compiler implicitly extend
 the lifetime of a lambda called by cilk spawn?  In any case, we really don't
 want to do this for all calls to unnamed lambdas just because we turned on
 cilk mode.
 

I have fixed this issue.  My function to map the variable's context from the 
spawner to the spawn helper function was going into the lambda function. I made 
it stop by adding a language specific copy_tree_body (basically stop going into 
the lambda function's body for C++ and for the rest of the times just use 
copy_tree_body_r, no code duplicating  is done between the two) that and it 
works fine now.

The fixed patch is attached and here are the fixed ChangeLogs:

gcc/c-family/ChangeLog
2013-11-25  Balaji V. Iyer  balaji.v.i...@intel.com

* cilk.c (cilk_outline): Replaced a call to copy_tree_body_r with the
language specific one.

gcc/c/ChangeLog
2013-11-25  Balaji V. Iyer  balaji.v.i...@intel.com

* c-objc-common.h (LANG_HOOKS_CILKPLUS_CILK_SPEC_COPY_BODY): New
define.

gcc/ChangeLog
2013-11-25  Balaji V. Iyer  balaji.v.i...@intel.com

* langhooks.h (lang_hooks_for_cilkplus::cilk_specific_copy_tree_body):
New field.
* langhooks-def.h
(LANG_HOOKS_CILKPLUS::LANG_HOOKS_CILKPLUS_CILK_SPEC_COPY_BODY):
Likewise.
(LANG_HOOKS_CILKPLUS_CILK_SPEC_COPY_BODY): New #define.

gcc/cp/ChangeLog
2013-11-25  Balaji V. Iyer  balaji.v.i...@intel.com

* cp-tree.h (cilk_valid_spawn): New prototype.
(gimplify_cilk_spawn): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(cilk_create_lambda_fn_tmp_var): Likewise.
(create_try_catch_expr): Likewise.
* decl.c (finish_function): Insert Cilk function-calls when a
_Cilk_spawn is used in a function.
* parser.c (cp_parser_postfix_expression): Added RID_CILK_SPAWN and
RID_CILK_SYNC cases

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/21/2013 05:40 PM, Iyer, Balaji V wrote:

+/* Returns a TRY_CATCH_EXPR that will encapsulate BODY, EXCEPT_DATA and
+   EXCEPT_FLAG.  */
+
+tree
+create_cilk_try_catch (tree except_flag, tree except_data, tree body)
+{
+  tree catch_list = alloc_stmt_list ();
+  append_to_statement_list (except_flag, catch_list);
+  append_to_statement_list (except_data, catch_list);
+  append_to_statement_list (do_begin_catch (), catch_list);
+  append_to_statement_list (build_throw (NULL_TREE), catch_list);
+  tree catch_tf_expr = build_stmt (EXPR_LOCATION (body), TRY_FINALLY_EXPR,
+  catch_list, do_end_catch (NULL_TREE));
+  catch_list = build2 (CATCH_EXPR, void_type_node, NULL_TREE,
+  catch_tf_expr);
+  tree try_catch_expr = build_stmt (EXPR_LOCATION (body), TRY_CATCH_EXPR,
+   body, catch_list);
+  return try_catch_expr;
+}


I had in mind something less cilk-specific: a function that takes two 
tree operands, one for the body and one for the throwing path. 
Basically, make catch_list a parameter and move the first two appends 
back into the calling function.



The reason is that, when you have something like this:

_Cilk_spawn [=]  { body } ();

I need to capture the function call (which in this case is the whole function) 
and throw it into a nested function.  The nested function implementation is 
shared with C. If the function is stored in a variable then I can just send 
that out to the nested function. I have added another constraint to make sure 
the function is a spawning function, this way we can reduce more cases were 
they are stored to a variable. The reason why I added this check in 
finish_call_expr is that it seemed to be most straight-forward for me and only 
place where I could do with least disruption (code-changes).


It looks like you're transforming any

[...] {...} (...);

into

auto lambda = [...]{...};
lambda(...);

which has significantly different semantics, particularly in terms of 
the lifetime of the lambda object.  In some of the Cilk online 
documentation, I see:



When spawning named lambda functions, be careful that the lifespan of the 
lambda extends at least until the next sync, or else the destructor for the 
lambda will race with the spawned call. For example:
double i = g();
if (some condition) {
   // named lambda with value capture of i
   auto f = [=i]() { double d = sin(i); f(d); };
   cilk_spawn f();
} // Ouch! destructor for f is in parallel with spawned call.


This would seem to apply even more to unnamed lambda functions, since 
normally they would be destroyed at the end of the full-expression, 
which must always be before the sync.  Does the Intel compiler 
implicitly extend the lifetime of a lambda called by cilk spawn?  In any 
case, we really don't want to do this for all calls to unnamed lambdas 
just because we turned on cilk mode.


Jason

RE: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

Hi Jason,
Please see my responses below. I have also attached a fixed patch and 
the Changelog entries are cut and pasted below.

 -Original Message-
 From: Jason Merrill [mailto:ja...@redhat.com]
 Sent: Thursday, November 21, 2013 1:59 PM
 To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
 Cc: Jeff Law
 Subject: Re: _Cilk_spawn and _Cilk_sync for C++
 
 On 11/17/2013 10:19 PM, Iyer, Balaji V wrote:
 cp/cp-cilkplus.o \
  - cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o \
  + cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o cp/cp-cilk.o \
 
 It seems unnecessary to have both cp-cilk.c and cp-cilkplus.c.  Please
 combine them.

Fixed. I removed cp-cilk.c and moved my work to cp-cilkplus.c. This will change 
my _Cilk_for for C++ work also, since I used cp-cilk.c to store all my 
routines. I will send out a fixed patch soon.

 
  +  extern tree do_begin_catch (void);
  +  extern tree do_end_catch (tree);
 
 If you want to use these, they need to be declared in cp-tree.h, not within
 another function.  Or better yet, factor out this code:
 

Fixed. I created a new function called cilk_create_try_catch () in cp/except.c

  +  append_to_statement_list (do_begin_catch (), catch_list);
  +  append_to_statement_list (build_throw (NULL_TREE), catch_list);
  +  tree catch_tf_expr = build_stmt (EXPR_LOCATION (body),
 TRY_FINALLY_EXPR,
  +  catch_list, do_end_catch (NULL_TREE));
  +  catch_list = build2 (CATCH_EXPR, void_type_node, NULL_TREE,
  +  catch_tf_expr);
  +  tree try_catch_expr = build_stmt (EXPR_LOCATION (body),
 TRY_CATCH_EXPR,
  +   body, catch_list);
 
 ...into a function in cp/except.c.
 

Yep, this is what I did.

  +  tree try_finally_expr = build_stmt (EXPR_LOCATION (body),
  + TRY_FINALLY_EXPR,
  +try_catch_expr, dtor);
  +  append_to_statement_list (try_finally_expr, list);
  +}
  +  else
  +append_to_statement_list (build_stmt (EXPR_LOCATION (body),
  + TRY_FINALLY_EXPR, body, dtor),
 list);
 
 This bit could be shared between the two branches.

Fixed. 

 
  +  /* When Cilk Plus is enabled, the lambda function need to be stored to
  + a variable because if the function is spawned, then we need some kind
  + of a handle.  */
  +  if (flag_enable_cilkplus  cxx_dialect = cxx0x
  +   TREE_CODE (fn) != VAR_DECL  TREE_CODE (fn) != OVERLOAD
  +   TREE_CODE (fn) != FUNCTION_DECL)
  +fn = cilk_create_lambda_fn_tmp_var (fn);
 
 I don't like making this change here.  What do you need a handle for?
 Why can't build_cilk_spawn deal with it?
 

The reason is that, when you have something like this:

_Cilk_spawn [=]  { body } ();

I need to capture the function call (which in this case is the whole function) 
and throw it into a nested function.  The nested function implementation is 
shared with C. If the function is stored in a variable then I can just send 
that out to the nested function. I have added another constraint to make sure 
the function is a spawning function, this way we can reduce more cases were 
they are stored to a variable. The reason why I added this check in 
finish_call_expr is that it seemed to be most straight-forward for me and only 
place where I could do with least disruption (code-changes).

  +case CILK_SPAWN_STMT:
  +  if (!potential_constant_expression_1 (CILK_SPAWN_FN (t), true,
 flags))
  +   return false;
  +  return true;
 
 Isn't Cilk spawn itself is non-constant, so you can just return false?
 

Fixed.

Here are the ChangeLog entries:

gcc/cp/ChangeLog
2013-11-21  Balaji V. Iyer  balaji.v.i...@intel.com

* cp-tree.h (cilk_valid_spawn): New prototype.
(gimplify_cilk_spawn): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(cilk_create_lambda_fn_tmp_var): Likewise.
(create_cilk_try_catch): Likewise.
* decl.c (finish_function): Insert Cilk function-calls when a
_Cilk_spawn is used in a function.
* parser.c (cp_parser_postfix_expression): Added RID_CILK_SPAWN and
RID_CILK_SYNC cases.
* cp-cilkplus.c (set_cilk_except_flag): New function.
(set_cilk_except_data): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(cilk_create_lambda_fn_tmp_var): Likewise.
* except.c (create_cilk_try_catch): Likewise.
* parser.h (IN_CILK_SPAWN): New #define.
* cp-objcp-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): Likewise.
(LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
* pt.c (tsubst_expr): Added CILK_SPAWN_STMT and CILK_SYNC_STMT cases.
* semantics.c (potential_constant_expression_1): Likewise.
(finish_call_expr): Stored the lambda function to a variable when Cilk
Plus

Re: _Cilk_spawn and _Cilk_sync for C++

[Jason Merrill]

On 11/17/2013 10:19 PM, Iyer, Balaji V wrote:

   cp/cp-cilkplus.o \
- cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o \
+ cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o cp/cp-cilk.o \


It seems unnecessary to have both cp-cilk.c and cp-cilkplus.c.  Please 
combine them.



+  extern tree do_begin_catch (void);
+  extern tree do_end_catch (tree);


If you want to use these, they need to be declared in cp-tree.h, not 
within another function.  Or better yet, factor out this code:



+  append_to_statement_list (do_begin_catch (), catch_list);
+  append_to_statement_list (build_throw (NULL_TREE), catch_list);
+  tree catch_tf_expr = build_stmt (EXPR_LOCATION (body), TRY_FINALLY_EXPR,
+  catch_list, do_end_catch (NULL_TREE));
+  catch_list = build2 (CATCH_EXPR, void_type_node, NULL_TREE,
+  catch_tf_expr);
+  tree try_catch_expr = build_stmt (EXPR_LOCATION (body), TRY_CATCH_EXPR,
+   body, catch_list);


...into a function in cp/except.c.


+  tree try_finally_expr = build_stmt (EXPR_LOCATION (body),
+ TRY_FINALLY_EXPR,
+try_catch_expr, dtor);
+  append_to_statement_list (try_finally_expr, list);
+}
+  else
+append_to_statement_list (build_stmt (EXPR_LOCATION (body),
+ TRY_FINALLY_EXPR, body, dtor), list);


This bit could be shared between the two branches.


+  /* When Cilk Plus is enabled, the lambda function need to be stored to
+ a variable because if the function is spawned, then we need some kind
+ of a handle.  */
+  if (flag_enable_cilkplus  cxx_dialect = cxx0x
+   TREE_CODE (fn) != VAR_DECL  TREE_CODE (fn) != OVERLOAD
+   TREE_CODE (fn) != FUNCTION_DECL)
+fn = cilk_create_lambda_fn_tmp_var (fn);


I don't like making this change here.  What do you need a handle for? 
Why can't build_cilk_spawn deal with it?



+case CILK_SPAWN_STMT:
+  if (!potential_constant_expression_1 (CILK_SPAWN_FN (t), true, flags))
+   return false;
+  return true;


Isn't Cilk spawn itself is non-constant, so you can just return false?

Jason

[PING]: _Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

Hello Everyone,
This patch was originally submitted October and then I refreshed it 
with trunk and submitted again last week. Did anyone get a chance to review 
this? Most of the changes are just in parsing and almost all of the processing 
is done in the middle part, which is already approved.

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Iyer, Balaji V
 Sent: Sunday, November 17, 2013 10:19 PM
 To: gcc-patches@gcc.gnu.org
 Cc: Jason Merrill (ja...@redhat.com); Jeff Law
 Subject: _Cilk_spawn and _Cilk_sync for C++
 
 Hello Jason et al.,
   Mike Stump mentioned that my _Cilk_spawn and _Cilk_sync for C++
 may have been lost in the email pile. So, attached is an updated _Cilk_spawn
 and _Cilk_sync for C++ patch. Is this Ok to install?
 
 Here are the ChangeLog entries (they shouldn't have changed since the last
 submission):
 
 gcc/cp/ChangeLog
 2013-11-17  Balaji V. Iyer  balaji.v.i...@intel.com
 
 * Make-lang.in (CXX_AND_OBJCXX_OBJS): Added cp/cp-cilk.o.
 * cp-cilk.c: New file.
 * cp-tree.h (cilk_valid_spawn): New prototype.
 (gimplify_cilk_spawn): Likewise.
 (cp_cilk_install_body_wframe_cleanup): Likewise.
 (cilk_create_lambda_fn_tmp_var): Likewise.
 * decl.c (finish_function): Insert Cilk function-calls when a
 _Cilk_spawn is used in a function.
 * except.c (do_begin_catch): Made the function non-static.
 (do_end_catch): Likewise.
 * parser.c (cp_parser_postfix_expression): Added RID_CILK_SPAWN and
 RID_CILK_SYNC cases.
 * parser.h (IN_CILK_SPAWN): New #define.
 * cp-objcp-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN):
 Likewise.
 (LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
 (LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
 * pt.c (tsubst_expr): Added CILK_SPAWN_STMT and CILK_SYNC_STMT
 cases.
 * semantics.c (potential_constant_expression_1): Likewise.
 (finish_call_expr): Stored the lambda function to a variable when Cilk
 Plus is enabled.
 * typeck.c (cp_build_compound_expr): Reject a spawned function in a
 compound expression.
 (check_return_expr): Reject a spawned function in a return expression.
 
 gcc/testsuite/ChangeLog
 2013-11-17  Balaji V. Iyer  balaji.v.i...@intel.com
 
 * g++.dg/cilk-plus/CK/catch_exc.cc: New test case.
 * g++.dg/cilk-plus/CK/const_spawn.cc: Likewise.
 * g++.dg/cilk-plus/CK/fib-opr-overload.cc: Likewise.
 * g++.dg/cilk-plus/CK/fib-tplt.cc: Likewise.
 * g++.dg/cilk-plus/CK/lambda_spawns.cc: Likewise.
 * g++.dg/cilk-plus/CK/lambda_spawns_tplt.cc: Likewise.
 * g++.dg/cilk-plus/cilk-plus.exp: Added support to run Cilk Keywords
 test stored in c-c++-common.  Also, added the Cilk runtime's library
 to the ld_library_path.
 
 Thanks,
 
 Balaji V. Iyer.j
diff --git a/gcc/cp/Make-lang.in b/gcc/cp/Make-lang.in
index 424f2e6..e046ee3 100644
--- a/gcc/cp/Make-lang.in
+++ b/gcc/cp/Make-lang.in
@@ -77,7 +77,7 @@ CXX_AND_OBJCXX_OBJS = cp/call.o cp/decl.o cp/expr.o cp/pt.o 
cp/typeck2.o \
  cp/search.o cp/semantics.o cp/tree.o cp/repo.o cp/dump.o cp/optimize.o \
  cp/mangle.o cp/cp-objcp-common.o cp/name-lookup.o cp/cxx-pretty-print.o \
  cp/cp-cilkplus.o \
- cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o \
+ cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o cp/cp-cilk.o \
  cp/vtable-class-hierarchy.o $(CXX_C_OBJS)
 
 # Language-specific object files for C++.
diff --git a/gcc/cp/cp-cilk.c b/gcc/cp/cp-cilk.c
new file mode 100644
index 000..0da95e8
--- /dev/null
+++ b/gcc/cp/cp-cilk.c
@@ -0,0 +1,118 @@
+/* Functions to handle Cilk keywords support in C++.
+   Copyright (C) 2011-2013  Free Software Foundation, Inc.
+   Contributed by Balaji V. Iyer balaji.v.i...@intel.com,
+   Intel Corporation.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   GCC is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GCC; see the file COPYING3.  If not see
+   http://www.gnu.org/licenses/.  */
+
+#include config.h
+#include system.h
+#include coretypes.h
+#include cp-tree.h
+#include tree-iterator.h
+#include cilk.h
+
+/* Sets the EXCEPTION bit (0x10) in the FRAME.flags field.  */
+
+static tree
+set_cilk_except_flag (tree frame)
+{
+  tree flags = cilk_dot (frame, CILK_TI_FRAME_FLAGS, 0);
+
+  flags = build2 (MODIFY_EXPR, void_type_node, flags,
+ build2 (BIT_IOR_EXPR, TREE_TYPE (flags), flags

_Cilk_spawn and _Cilk_sync for C++

[Iyer, Balaji V]

Hello Jason et al.,
Mike Stump mentioned that my _Cilk_spawn and _Cilk_sync for C++ may 
have been lost in the email pile. So, attached is an updated _Cilk_spawn and 
_Cilk_sync for C++ patch. Is this Ok to install?

Here are the ChangeLog entries (they shouldn't have changed since the last 
submission):

gcc/cp/ChangeLog
2013-11-17  Balaji V. Iyer  balaji.v.i...@intel.com

* Make-lang.in (CXX_AND_OBJCXX_OBJS): Added cp/cp-cilk.o.
* cp-cilk.c: New file.
* cp-tree.h (cilk_valid_spawn): New prototype.
(gimplify_cilk_spawn): Likewise.
(cp_cilk_install_body_wframe_cleanup): Likewise.
(cilk_create_lambda_fn_tmp_var): Likewise.
* decl.c (finish_function): Insert Cilk function-calls when a
_Cilk_spawn is used in a function.
* except.c (do_begin_catch): Made the function non-static.
(do_end_catch): Likewise.
* parser.c (cp_parser_postfix_expression): Added RID_CILK_SPAWN and
RID_CILK_SYNC cases.
* parser.h (IN_CILK_SPAWN): New #define.
* cp-objcp-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): Likewise.
(LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
* pt.c (tsubst_expr): Added CILK_SPAWN_STMT and CILK_SYNC_STMT cases.
* semantics.c (potential_constant_expression_1): Likewise.
(finish_call_expr): Stored the lambda function to a variable when Cilk
Plus is enabled.
* typeck.c (cp_build_compound_expr): Reject a spawned function in a
compound expression.
(check_return_expr): Reject a spawned function in a return expression.

gcc/testsuite/ChangeLog
2013-11-17  Balaji V. Iyer  balaji.v.i...@intel.com

* g++.dg/cilk-plus/CK/catch_exc.cc: New test case.
* g++.dg/cilk-plus/CK/const_spawn.cc: Likewise.
* g++.dg/cilk-plus/CK/fib-opr-overload.cc: Likewise.
* g++.dg/cilk-plus/CK/fib-tplt.cc: Likewise.
* g++.dg/cilk-plus/CK/lambda_spawns.cc: Likewise.
* g++.dg/cilk-plus/CK/lambda_spawns_tplt.cc: Likewise.
* g++.dg/cilk-plus/cilk-plus.exp: Added support to run Cilk Keywords
test stored in c-c++-common.  Also, added the Cilk runtime's library
to the ld_library_path.

Thanks,

Balaji V. Iyer.j
diff --git a/gcc/cp/Make-lang.in b/gcc/cp/Make-lang.in
index 424f2e6..e046ee3 100644
--- a/gcc/cp/Make-lang.in
+++ b/gcc/cp/Make-lang.in
@@ -77,7 +77,7 @@ CXX_AND_OBJCXX_OBJS = cp/call.o cp/decl.o cp/expr.o cp/pt.o 
cp/typeck2.o \
  cp/search.o cp/semantics.o cp/tree.o cp/repo.o cp/dump.o cp/optimize.o \
  cp/mangle.o cp/cp-objcp-common.o cp/name-lookup.o cp/cxx-pretty-print.o \
  cp/cp-cilkplus.o \
- cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o \
+ cp/cp-gimplify.o cp/cp-array-notation.o cp/lambda.o cp/cp-cilk.o \
  cp/vtable-class-hierarchy.o $(CXX_C_OBJS)
 
 # Language-specific object files for C++.
diff --git a/gcc/cp/cp-cilk.c b/gcc/cp/cp-cilk.c
new file mode 100644
index 000..0da95e8
--- /dev/null
+++ b/gcc/cp/cp-cilk.c
@@ -0,0 +1,118 @@
+/* Functions to handle Cilk keywords support in C++.
+   Copyright (C) 2011-2013  Free Software Foundation, Inc.
+   Contributed by Balaji V. Iyer balaji.v.i...@intel.com,
+   Intel Corporation.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   GCC is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GCC; see the file COPYING3.  If not see
+   http://www.gnu.org/licenses/.  */
+
+#include config.h
+#include system.h
+#include coretypes.h
+#include cp-tree.h
+#include tree-iterator.h
+#include cilk.h
+
+/* Sets the EXCEPTION bit (0x10) in the FRAME.flags field.  */
+
+static tree
+set_cilk_except_flag (tree frame)
+{
+  tree flags = cilk_dot (frame, CILK_TI_FRAME_FLAGS, 0);
+
+  flags = build2 (MODIFY_EXPR, void_type_node, flags,
+ build2 (BIT_IOR_EXPR, TREE_TYPE (flags), flags,
+ build_int_cst (TREE_TYPE (flags),
+CILK_FRAME_EXCEPTING)));
+  return flags;
+}
+
+/* Sets the frame.EXCEPT_DATA field to the head of the exception pointer.  */
+
+static tree
+set_cilk_except_data (tree frame)
+{
+  tree except_data = cilk_dot (frame, CILK_TI_FRAME_EXCEPTION, 0);
+  tree uresume_fn = builtin_decl_implicit (BUILT_IN_EH_POINTER);
+  tree ret_expr;
+  uresume_fn  = build_call_expr (uresume_fn, 1,
+build_int_cst (integer_type_node, 0));
+  ret_expr = build2 (MODIFY_EXPR, void_type_node

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jakub Jelinek]

On Mon, Oct 28, 2013 at 09:01:45PM +, Iyer, Balaji V wrote:
 Thanks! I will extract and check in the Cilk_spawn and _Cilk_sync for C work.

This broke bootstrap on i686-linux, fixed thusly, committed as obvious:

2013-10-30  Jakub Jelinek  ja...@redhat.com

* cilk.c (create_cilk_helper_decl): Use HOST_WIDE_INT_PRINT_DEC.

--- gcc/c-family/cilk.c.jj  2013-10-30 13:53:52.0 +0100
+++ gcc/c-family/cilk.c 2013-10-30 14:44:49.358912539 +0100
@@ -287,9 +287,9 @@ create_cilk_helper_decl (struct wrapper_
 {
   char name[20];
   if (wd-type == CILK_BLOCK_FOR)
-sprintf (name, _cilk_for_%ld, cilk_wrapper_count++);
+sprintf (name, _cilk_for_ HOST_WIDE_INT_PRINT_DEC, cilk_wrapper_count++);
   else if (wd-type == CILK_BLOCK_SPAWN)
-sprintf (name, _cilk_spn_%ld, cilk_wrapper_count++);
+sprintf (name, _cilk_spn_ HOST_WIDE_INT_PRINT_DEC, cilk_wrapper_count++);
   else
 gcc_unreachable (); 
   


Jakub

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 10/22/13 10:22, Iyer, Balaji V wrote:

Hi Jeff, I have attached 2 patches - 1 for C and 1 for C++ - along
with the changelogs (ChangeLog.cilkplus for C and common changes,
cp-ChangeLog.cilkplus for C++ specific files) with the changes you
have requested.  Answers to your questions are given below also.

It passes all its tests and doesn't affect any other existing tests
(i.e. by affect I mean fail a passing test or pass a failing test).



A note.  cilk-common seems to be a mix of tree and rtl bits.  Those may 
need to be broken into separate files and/or moved around as part of the 
reorganization work that's occurring in GCC right now.  So if Andrew or 
someone else pings you, please respond appropriately.


The C (and shared) parts of this patch are OK.  Jason needs to review 
the C++ front-end changes.


If possible, I recommend installing the C (and shared) bits as well as 
the runtime component if they can build  work w/o the C++ front-end 
changes, then track the C++ front-end changes separately.


Jeff

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

 -Original Message-
 From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
 ow...@gcc.gnu.org] On Behalf Of Jeff Law
 Sent: Monday, October 28, 2013 4:24 PM
 To: Iyer, Balaji V; r...@redhat.com; Jason Merrill (ja...@redhat.com); Aldy
 Hernandez (al...@redhat.com)
 Cc: gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 On 10/22/13 10:22, Iyer, Balaji V wrote:
  Hi Jeff, I have attached 2 patches - 1 for C and 1 for C++ - along
  with the changelogs (ChangeLog.cilkplus for C and common changes,
  cp-ChangeLog.cilkplus for C++ specific files) with the changes you
  have requested.  Answers to your questions are given below also.
 
  It passes all its tests and doesn't affect any other existing tests
  (i.e. by affect I mean fail a passing test or pass a failing test).
 
 
 A note.  cilk-common seems to be a mix of tree and rtl bits.  Those may need 
 to
 be broken into separate files and/or moved around as part of the 
 reorganization
 work that's occurring in GCC right now.  So if Andrew or someone else pings
 you, please respond appropriately.
 
 The C (and shared) parts of this patch are OK.  Jason needs to review the C++
 front-end changes.
 
 If possible, I recommend installing the C (and shared) bits as well as the 
 runtime
 component if they can build  work w/o the C++ front-end changes, then track
 the C++ front-end changes separately.

Thanks! I will extract and check in the Cilk_spawn and _Cilk_sync for C work.

-Balaji V. Iyer.

 
 Jeff

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

 
  Can you take a look at calls.c::special_function_p and determine if we need
 to
  do something special for spawn here?
 
 
  I will look into it and let you know.
 Any word on this?
 

Hi Jeff,
I looked into this function and from what I can tell, it is used to 
mark certain functions (e.g. builtin functions) as special and thus don't do 
special optimizations on them like a regular function.  The thing is, the 
spawnee (the function being spawned) can be pretty much any regular function. 
The compiler doesn't even touch inside the function. The compiler inserts 
specific Cilk function calls in the spawner and transplants the function . The 
only major restriction I know is that the frame pointer needs to be used and 
that I mark as I mentioned above.  

Is there anything you were thinking about that I missed?

Thanks,

Balaji V. Iyer.


 jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 10/23/13 13:46, Iyer, Balaji V wrote:



Can you take a look at calls.c::special_function_p and
determine if we need

to

do something special for spawn here?



I will look into it and let you know.

Any word on this?



Hi Jeff, I looked into this function and from what I can tell, it is
used to mark certain functions (e.g. builtin functions) as special
and thus don't do special optimizations on them like a regular
function.  The thing is, the spawnee (the function being spawned) can
be pretty much any regular function. The compiler doesn't even touch
inside the function. The compiler inserts specific Cilk function
calls in the spawner and transplants the function . The only major
restriction I know is that the frame pointer needs to be used and
that I mark as I mentioned above.

Is there anything you were thinking about that I missed?
There wasn't anything in particular I was worried about.  Just a general 
question as to whether or not we needed to mark the spawner or spawnee 
as special, partiuclarly returns twice (setjmp/fork) and never returns 
(longjmp).


Jeff

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

 -Original Message-
 From: Jeff Law [mailto:l...@redhat.com]
 Sent: Wednesday, October 23, 2013 3:53 PM
 To: Iyer, Balaji V; r...@redhat.com; Jason Merrill (ja...@redhat.com); Aldy
 Hernandez (al...@redhat.com)
 Cc: gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 On 10/23/13 13:46, Iyer, Balaji V wrote:
 
  Can you take a look at calls.c::special_function_p and determine if
  we need
  to
  do something special for spawn here?
 
 
  I will look into it and let you know.
  Any word on this?
 
 
  Hi Jeff, I looked into this function and from what I can tell, it is
  used to mark certain functions (e.g. builtin functions) as special and
  thus don't do special optimizations on them like a regular function.
  The thing is, the spawnee (the function being spawned) can be pretty
  much any regular function. The compiler doesn't even touch inside the
  function. The compiler inserts specific Cilk function calls in the
  spawner and transplants the function . The only major restriction I
  know is that the frame pointer needs to be used and that I mark as I
  mentioned above.
 
  Is there anything you were thinking about that I missed?
 There wasn't anything in particular I was worried about.  Just a general 
 question
 as to whether or not we needed to mark the spawner or spawnee as special,
 partiuclarly returns twice (setjmp/fork) and never returns (longjmp).
 
I do check for those in the the spawnee using the check_outlined_calls function.

 Jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 10/16/13 15:49, Iyer, Balaji V wrote:

In ira.c:

+   /* We need a frame pointer for all Cilk Plus functions that use
+ Cilk keywords.  */
+   || (flag_enable_cilkplus  cfun-is_cilk_function)
Can you explain to me a bit more why you need a frame pointer?  I'm trying to
determine if it's best to leave this as-is or have this code detect a property 
in the
generated code for the function.  From a modularity standpoint it seems pretty
gross that we have to peek at this within IRA.



Cilk Runtime functions changes the stack pointer. So, frame pointer is 
necessary.
Nevermind -- that seems to be the location where this is detected now. 
So this is fine.







In a couple places I saw this comment:
+  /* Cilk keywords currently need to replace some variables that
+ ordinary nested functions do not.  */  bool remap_var_for_cilk;
I didn't see anywhere that explained exactly why some variables that do not
ordinarily need replacing need replacing when cilk is enabled.  If it's in the 
patch
somewhere, just point me to it. If not, add documentation about why these
variables need remapping for cilk.



It is used in the cilk_outline function.
Thanks.  Presumably the comment We don't want the private variables 
anymore is the relevant code/comment?



Does anything actually ensure we don't have multiple syncs?




Well, _Cilk_sync expands to something like this:

If (!sync_occurred)
__cilkrts_sync()

So, having multiple Cilk syncs doesn't harm, just that the then case of the 
if-statement will not be taken.

OK.  Thanks.





What's the thinking behind parsing calls to cilk_spawn as a normal call if 
there's
an error?  Referring to this code in gimplify.c:
+   case CILK_SPAWN_STMT:
+ gcc_assert
+   (fn_contains_cilk_spawn_p (cfun)
+ lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
+ if (!seen_error ())
+   {
+ ret = (enum gimplify_status)
+   lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
+post_p);
+ break;
+   }
+ /* If errors are seen, then just process it as a CALL_EXPR.
+ */
+



Well, if there is an error the compiler is not going to produce an executable. 
So, I just let the compiler go far as it can and catch all the other errors. If 
the error is cilk related, we have already called them out on it. Adding 
_Cilk_spawn specific routines would add additional complication.


I guess that's a reasonable fallback position in case of an error.


Meta-question, when we're not in cilk mode, should we be consuming the cilk
tokens?  I'm not familiar at all with our parser, so I'm not sure if we can 
handle
this gracefully.  Though I guess parsing hte token and warning/error if not in 
Cilk
mode is probably the best course of action.



In the compiler, I couldn't make conditional tokens. When the parser hits a 
_Cilk_spawn or _Cilk_sync token, it will check if Cilk Plus is enabled or will 
complain. Now that I think about it in detail, I suppose it will also block if 
anyone wants to have a variable name called _Cilk_spawn or _Cilk_sync and not 
using -fcilkplus. But, they start with '_', and so I guess it is not a normal 
case.
Figured it was ugly at best to avoid consuming the cilk tokens when not 
in cilk mode.





Can you take a look at calls.c::special_function_p and determine if we need to
do something special for spawn here?



I will look into it and let you know.

Any word on this?

jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 10/18/13 15:06, Iyer, Balaji V wrote:

Hi Jeff, Please see my comments below. Also, I am adding all these
changes to the files as you requested in my local directory. Should I
send you an updated patch along the way?
I'll let you know when I've worked my way through everything.  ISTM an 
updated patch after that would be best.  I'm hoping to get through the 
remaining changes today, then review your replies in detail.



jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 10/18/13 15:06, Iyer, Balaji V wrote:


The main reason why I made it volatile (as expressed by the volatil
bool variable) is that I want to make sure these values aren't
optimized by the compiler and the value is fetched from memory on
every access. I have added an explanation to the header comment.
I figured that was the case; it's also what origianlly got me thinking 
about memory models.  volatile can help with getting the right number of 
accesses (though there are cases where it gets in the way).  But 
volatile is not a fence/barrier :-)




Presumably users never concern themselves with the cilkrts_pedigree
structure, it's entirely hidden, right?  So there's no way for a
user to ask for an array of these things, right?
Ok, so this relates to the question about keeping all the structures in 
sync






I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems
excessive. Why not compute the natural alignment for that structure
and use that? Similarly for the cilkrts_stack_frame, except that it
seems to use PREFERRED_STACK_BOUNDARY.  Is there some reason why
each shouldn't be aligned on whatever boundary the target would
normally align those structures?



Fixed, I now let the compiler set these values
Actually, thinking more about this, we've got to make sure we're 
compatible with ICC on this stuff.  So if you need to weak the 
alignments to be ICC compatible, then, well, we have to do it.   If (for 
example) the compilers differ on their notion of the structure's 
alignment, then you couldn't make an array of them and use it reliably 
across the ICC  GCC implementations.


You might consider some sort of interoperatibity test.  Obviously the 
test wouldn't run if ICC wasn't installed, but having a test to catch 
this stuff early (if it ever happens) would be wise.



These structures don't change often and whenever they do (i.e. fields
get added into it) it is kind of a big deal.  So far, when we have
changed - once in the past 3 yrs. or so - we have allowed backward
compatibility.
Good to hear it's a big deal.  Once there are multiple implementations 
in the wild relying upon them, it'll be an even bigger deal..





We've given it some thought, but have neither the manpower nor
charter to port Cilk to non-Intel architectures.  We've done a trial
port to ARM to prove it could be done, but we ran fib (a small
example) once and declared victory.  We're sure that more work needs
to be done here and would welcome changes to the runtime to support
other architectures.  In particular, most or all uses of
assembly-language instructions should be replaced by compiler
intrinsics and memory barriers probably need to be added for
architectures that have a more relaxed memory model than the x86.
Our hope is that, once Cilk Plus was added to gcc, that members of
the community would help us port the runtime to other architectures.
Such ports could probably start with full barriers, it'd be painful from 
a performance standpoint, but as a starting point, sufficient.


jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 09/11/13 12:18, Iyer, Balaji V wrote:

Hello Everyone, Couple weeks back, I had submitted a patch for review
that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
the C compiler. I recently finished C++ implementation also. In this
email, I am attaching 2 patches: 1 for C (and the common parts for C
and C++) and 1 for C++. The C++ Changelog is labelled
cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
There isn't much changes in the C patch. Only noticeable changes
would be moving functions to the common parts so that C++ can use
them.

It passes all the tests and does not affect  (by affect I mean fail a
passing test or pass a failing one) any of the other tests in the
testsuite directory.

Is this Ok for trunk?
Here are my final notes from this pass over the changes.  If you could 
send an updated patch to the list, it would be appreciated.  I may have 
more comments now that I've looked over everything and have a slightly 
better understanding of the overall structure.



I'm not a C++ guy, but are you going to have to do anything special with 
lambdas?  Such as in cilk_set_spawn_marker?


I'm going to assume you don't need to do anything for the C++ specific 
decls in copy_decl_for_cilk.  Note I didn't look at any of the C++ 
specific files, so if you're handling it there, that's fine with me.


I didn't look at the tests closely.  How thorough are those tests, 
particularly for front-end issues?  The reason I ask is those tests will 
help ensure that folks don't break the cilk+ support as often as they 
might otherwise.  Basically they cover for the lack of knowledge about 
the cilk+ implementation by providing developers a heads-up that they 
broke something.  Are there any internal testsuites Intel could 
contribute to help beef up testing?


Can you #undef the helper macros SUBTREE, MODIFIED, INITIALIZED that are 
defined in extract_free_variables?  A nit, but those kind of defines can 
certainly surprised folks.  Actually, unless there's a compelling 
reason, why not just go ahead and make the calls to 
extract_free_variables explicit and drop the macros completely?


In all, I didn't see anything that made me say wait, this is a huge 
issue we need to address.  Presumably you and Aldy worked through those 
before I got involved ;-)


jeff

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

Hi Jeff,
Please see my comments below. Also, I am adding all these changes to 
the files as you requested in my local directory. Should I send you an updated 
patch along the way?

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Jeff Law [mailto:l...@redhat.com]
 Sent: Thursday, October 17, 2013 5:30 PM
 To: Iyer, Balaji V; r...@redhat.com; Jason Merrill (ja...@redhat.com); Aldy
 Hernandez (al...@redhat.com)
 Cc: gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 On 09/11/13 12:18, Iyer, Balaji V wrote:
  Hello Everyone, Couple weeks back, I had submitted a patch for review
  that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
  the C compiler. I recently finished C++ implementation also. In this
  email, I am attaching 2 patches: 1 for C (and the common parts for C
  and C++) and 1 for C++. The C++ Changelog is labelled
  cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
  There isn't much changes in the C patch. Only noticeable changes would
  be moving functions to the common parts so that C++ can use them.
 
  It passes all the tests and does not affect  (by affect I mean fail a
  passing test or pass a failing one) any of the other tests in the
  testsuite directory.
 More random comments, questions  things to fix:
 
 In cilk_common.c we have:
 +/* Returns the value in structure FRAME pointed by the FIELD_NUMBER
 +   (e.g. X.y).
 +   FIELD_NUMBER is an index to the structure FRAME_PTR.  For details
 +   about these fields, refer to cilk_trees structure in cilk.h and
 +   cilk_init_builtins function  in this file.  Returns a TREE that is
 the type
 +   of the field represented by FIELD_NUMBER.  */
 +
 +tree
 +cilk_dot (tree frame, int field_number, bool volatil) {
 +  tree field = cilk_trees[field_number];
 +  field = fold_build3 (COMPONENT_REF, TREE_TYPE (field), frame, field,
 +  NULL_TREE);
 +  TREE_THIS_VOLATILE (field) = volatil;
 +  return field;
 +}
 
 There's no description of what the VOLATIL argument does.  Similarly for
 cilk_arrow.  From reading other code it appears to have the usual meaning of
 volatile, but it's best to be explicit.

The main reason why I made it volatile (as expressed by the volatil bool 
variable) is that I want to make sure these values aren't optimized by the 
compiler and the value is fetched from memory on every access. I have added an 
explanation to the header comment.

 
 In add_field:
 
 
 +/* This function will add FIELD of type TYPE to a defined built-in
 +   structure.  */
 +
 +static tree
 +add_field (const char *name, tree type, tree fields) {
 +  tree  t = get_identifier (name);
 +  tree field = build_decl (BUILTINS_LOCATION, FIELD_DECL, t, type);
 +  TREE_CHAIN (field) = fields;
 +  return field;
 +}
 NAME is not documented.  Nit: Just one space between the type specifier and
 the variable name  on this line:
 

Fixed both.

 tree  t = get_identifier (name);
 
 
 Presumably add_field is called well in advance of layout_decl :-)
 

Yes.

 install_builtin doesn't document CODE or PUBLISH in its comment header.
 

Fixed.

 
 Presumably users never concern themselves with the cilkrts_pedigree structure,
 it's entirely hidden, right?  So there's no way for a user to ask for an 
 array of
 these things, right?
 

A user can ignore pedigrees if they have no use for them (most don't), but 
__cilkrts_pedigree is specifically intended for read-only access by the user.  
A pedigree is a way of identifying where a parallel application is in the 
Directed Acyclic Graph that can be used to represent it. Regardless of the 
schedule chosen by the runtime, the pedigree will always be the same for the 
same point in the calculation. The runtime creates and maintains this 
structure, but does not use it internally for anything (unless the 
record/replay logic is turned on).  The pedigree is accessed using a public 
function, __cilkrts_get_pedigree(), in the cilk_api.h header and can be used, 
for example, to create deterministic parallel random number generators.

 
 I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive.
 Why not compute the natural alignment for that structure and use that?
 Similarly for the cilkrts_stack_frame, except that it seems to use
 PREFERRED_STACK_BOUNDARY.  Is there some reason why each shouldn't be
 aligned on whatever boundary the target would normally align those structures?
 

Fixed, I now let the compiler set these values

 For the ctx array, presumably it's some kind of context.  Where does the 
 size
 of that array come from?
 

Yes, it is context. Here is a link to the abi.h that describes fields in this 
structure: 
http://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libcilkrts/include/internal/abi.h;h=8f64b1bc5df8a0dad18cc14bb4e2cb51a60433e9;hb=7977b69ea6b3bb535aa85a757aee3d11d37d7ff0

 More generally is there any way we can ensure those structures are consistent
 between the runtime

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 09/11/13 12:18, Iyer, Balaji V wrote:

Hello Everyone, Couple weeks back, I had submitted a patch for review
that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
the C compiler. I recently finished C++ implementation also. In this
email, I am attaching 2 patches: 1 for C (and the common parts for C
and C++) and 1 for C++. The C++ Changelog is labelled
cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
There isn't much changes in the C patch. Only noticeable changes
would be moving functions to the common parts so that C++ can use
them.

It passes all the tests and does not affect  (by affect I mean fail a
passing test or pass a failing one) any of the other tests in the
testsuite directory.

More random comments, questions  things to fix:

In cilk_common.c we have:
+/* Returns the value in structure FRAME pointed by the FIELD_NUMBER
+   (e.g. X.y).
+   FIELD_NUMBER is an index to the structure FRAME_PTR.  For details
+   about these fields, refer to cilk_trees structure in cilk.h and
+   cilk_init_builtins function  in this file.  Returns a TREE that is 
the type

+   of the field represented by FIELD_NUMBER.  */
+
+tree
+cilk_dot (tree frame, int field_number, bool volatil)
+{
+  tree field = cilk_trees[field_number];
+  field = fold_build3 (COMPONENT_REF, TREE_TYPE (field), frame, field,
+  NULL_TREE);
+  TREE_THIS_VOLATILE (field) = volatil;
+  return field;
+}

There's no description of what the VOLATIL argument does.  Similarly for 
cilk_arrow.  From reading other code it appears to have the usual 
meaning of volatile, but it's best to be explicit.


In add_field:


+/* This function will add FIELD of type TYPE to a defined built-in
+   structure.  */
+
+static tree
+add_field (const char *name, tree type, tree fields)
+{
+  tree  t = get_identifier (name);
+  tree field = build_decl (BUILTINS_LOCATION, FIELD_DECL, t, type);
+  TREE_CHAIN (field) = fields;
+  return field;
+}
NAME is not documented.  Nit: Just one space between the type specifier 
and the variable name  on this line:


tree  t = get_identifier (name);


Presumably add_field is called well in advance of layout_decl :-)

install_builtin doesn't document CODE or PUBLISH in its comment header.


Presumably users never concern themselves with the cilkrts_pedigree 
structure, it's entirely hidden, right?  So there's no way for a user to 
ask for an array of these things, right?



I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive. 
Why not compute the natural alignment for that structure and use that? 
Similarly for the cilkrts_stack_frame, except that it seems to use 
PREFERRED_STACK_BOUNDARY.  Is there some reason why each shouldn't be 
aligned on whatever boundary the target would normally align those 
structures?


For the ctx array, presumably it's some kind of context.  Where does 
the size of that array come from?


More generally is there any way we can ensure those structures are 
consistent between the runtime and the compiler?  How often have those 
structures changed over time and how did y'all deal with the changes?



Presumably get_frame_arg is only used on calls into the cilk runtime, 
right and are all generated by GCC, and should always have the right 
number of arguments, should always be compatible, etc?!?  If so, then 
those sanity checks should probably be asserts.  Else it seems fine.



Comment for cilk_detach indicates it returns const0_rtx, but signature 
has void for the return type.


I'm assuming the code in expand_cilk_sync actually implements the 
pseudocode.  I didn't verify every hunk of tree you built.


I'd been wondering about concurrent access to various structures in 
here.  x86 has a fairly easy-to-use memory model.  Has any thought been 
given to what would need to change to support the weaker memory models 
such as are found on PPC?


More tomorrow I hope.

jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 09/11/13 12:18, Iyer, Balaji V wrote:

Hello Everyone, Couple weeks back, I had submitted a patch for review
that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
the C compiler. I recently finished C++ implementation also. In this
email, I am attaching 2 patches: 1 for C (and the common parts for C
and C++) and 1 for C++. The C++ Changelog is labelled
cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
There isn't much changes in the C patch. Only noticeable changes
would be moving functions to the common parts so that C++ can use
them.

It passes all the tests and does not affect  (by affect I mean fail a
passing test or pass a failing one) any of the other tests in the
testsuite directory.

Is this Ok for trunk?
This is not a complete review, but obviously I am starting to look at 
the patch and figured you could start addressing issues as I find them. 
 I usually start by trying to filter out all the stuff that is fairly 
benign, so I'm looking at code in a fairly random order.


You'll also note some items are just questions I'd like you to answer 
and don't (at this time) require any code changes -- they just help me 
understand everything.


I'm also not looking at the C++ bits -- my familiarity with the C++ 
front-end is minimal at best and I'd probably do more harm than good 
reviewing that code.




In gcc/Makefile.in, since the original submission of your patch we have 
integrated automatic dependency generation.  As a result several hunks 
to provide dependencies for cilk.o and update dependencies for other 
objects are no longer needed.  This is true for hte various Make-lang.in 
files as well.


builtins.c:

+  if (flag_enable_cilkplus  (!strcmp (name, __cilkrts_detach)
+  || !strcmp (name, __cilkrts_pop_frame)))
Formatting nit.
  if (fubar
   (com | baz))

in cppbuiltin.c, presumably the __cilk predefine is some kind of version 
#?  I'm going to assume that __clik is the same #define that ICC sets 
up, correct?


In function.h:

+  /* This will indicate whether a function is a cilk function */
+  unsigned int is_cilk_function : 1;

Doesn't this really mean calls into the cilk runtime?



In ira.c:

+   /* We need a frame pointer for all Cilk Plus functions that use
+ Cilk keywords.  */
+   || (flag_enable_cilkplus  cfun-is_cilk_function)
Can you explain to me a bit more why you need a frame pointer?  I'm 
trying to determine if it's best to leave this as-is or have this code 
detect a property in the generated code for the function.  From a 
modularity standpoint it seems pretty gross that we have to peek at this 
within IRA.




In a couple places I saw this comment:
+  /* Cilk keywords currently need to replace some variables that
+ ordinary nested functions do not.  */
+  bool remap_var_for_cilk;
I didn't see anywhere that explained exactly why some variables that do 
not ordinarily need replacing need replacing when cilk is enabled.  If 
it's in the patch somewhere, just point me to it. If not, add 
documentation about why these variables need remapping for cilk.



In gimplify.c:
+  /* Implicit _Cilk_sync must be inserted right before any return statement
+ if there is a _Cilk_spawn in the function.  If the user has provided a
+ _Cilk_sync, the optimizer should remove this duplicate one.  */
+  if (fn_contains_cilk_spawn_p (cfun))
+{
+  tree impl_sync = build0 (CILK_SYNC_STMT, void_type_node);
+  gimplify_and_add (impl_sync, pre_p);
+}
+
Does anything actually ensure we don't have multiple syncs?


What's the thinking behind parsing calls to cilk_spawn as a normal call 
if there's an error?  Referring to this code in gimplify.c:

+   case CILK_SPAWN_STMT:
+ gcc_assert
+   (fn_contains_cilk_spawn_p (cfun)
+ lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
+ if (!seen_error ())
+   {
+ ret = (enum gimplify_status)
+   lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
+post_p);
+ break;
+   }
+ /* If errors are seen, then just process it as a CALL_EXPR.  */
+

Meta-question, when we're not in cilk mode, should we be consuming the 
cilk tokens?  I'm not familiar at all with our parser, so I'm not sure 
if we can handle this gracefully.  Though I guess parsing hte token and 
warning/error if not in Cilk mode is probably the best course of action.


Can you take a look at calls.c::special_function_p and determine if we 
need to do something special for spawn here?



More tomorrow...

jeff

[PING][PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

Hello Everyone,
I submitted this patch ~1 months ago. Did anyone get a chance to look 
into this patch? 

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Iyer, Balaji V
 Sent: Wednesday, September 11, 2013 2:18 PM
 To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; Aldy
 Hernandez (al...@redhat.com)
 Cc: gcc-patches@gcc.gnu.org
 Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 Hello Everyone,
   Couple weeks back, I had submitted a patch for review that will
 implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into the C compiler. I
 recently finished C++ implementation also. In this email, I am attaching 2
 patches: 1 for C (and the common parts for C and C++) and 1 for C++. The C++
 Changelog is labelled cp-ChangeLog.cilkplus and the other one is just
 ChangeLog.cilkplus. There isn't much changes in the C patch. Only noticeable
 changes would be moving functions to the common parts so that C++ can use
 them.
 
   It passes all the tests and does not affect  (by affect I mean fail a 
 passing
 test or pass a failing one) any of the other tests in the testsuite directory.
 
   Is this Ok for trunk?
 
 Thanks,
 
 Balaji V. Iyer.
 
  -Original Message-
  From: Iyer, Balaji V
  Sent: Friday, August 30, 2013 1:02 PM
  To: gcc-patches@gcc.gnu.org
  Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
  The email seem to be bouncing gcc-patches. I have gzipped my patch.
 
  Thanks,
 
  Balaji V. Iyer.
 
 
-Original Message-
From: Iyer, Balaji V
Sent: Friday, August 30, 2013 11:42 AM
To: 'Aldy Hernandez'
Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
for C
   
Hi Aldy,
Attached, please find a fixed patch and the changelog entries.
   
 -Original Message-
 From: Aldy Hernandez [mailto:al...@redhat.com]
 Sent: Wednesday, August 28, 2013 2:36 PM
 To: Iyer, Balaji V
 Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
 for C

 On 08/27/13 16:27, Iyer, Balaji V wrote:
  Hello Aldy, I went through all the emails and here are the
  major issues that I could gather (other than lowering the
  keywords after gimplification, which I am skipping since it is
  more of an optimization for now).

 Ok, for now I am fine with delaying handling all this as a
 gimple tuple since most of your code lives in it's only little world 
 :).
 But I will go on record saying that part of the reason that you
 have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is
 because you don't
have easy gimplified code to examine.
 Anyways, agreed, you can do this later.

 
  1. Calling the gimplify_cilk_spawn on top of the gimplify_expr
  before the switch-statement could slow the compiler down 2. I
  need a CILK_SPAWN_STMT case in the switch statement in
  gimplify_expr
  (). 3.
  No test for catching the suspicious spawned function warning 4.
  Reasoning for expanding the 2 builtin functions in builtins.c
  instead of just inserting the appropriate expanded-code when I
  am inserting the function call.
 
  Did I miss anything else (or misunderstand anything you pointed 
  out)?
 
  Here are my answers to those questions above and am attaching
  a fixed patch with the changelog entries:
 
  1  2(partial): There are 3 places where we could have _Cilk_spawn:
  INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and
  MODIFY_EXPRS
are
  both gimplified using gimplify_modify_expr. I have moved the
  cilk_detect_spawn into this function. We will go into the
  cilk_detect_spawn if cilk plus is enabled, and if there is a
  cilk_frame (meaning the function has a Cilk_spawn in it)
  thereby reducing the number of hits into this function 
  significantly.
  Inside this function, it will go into the function that has a
  spawned function call and then unwrap the CILK_SPAWN_STMT
  wrapper and returns true. This shouldn't cause a huge
  compilation time
  hit. 2.
  To handle CALL_EXPR (e.g. _Cilk_spawn foo (x), where foo
  returns a void or the return value of it is ignored), I have
  added a CILK_SPAWN_STMT
case.
  Again, I am calling the detect_cilk_spawn and we will only
  step into this function if Cilk Plus is enabled and if there
  is a cilk-frame (i.e saying the function has a cilk spawn in
  it). If there is an error (seen_error () == true), then it
  just falls through into CALL_EXPR and is handled like a normal
  call expr not spawned expression. 3. This warning rarely get
  hit, and I have seen it hit

 See my comments below on this.

  only when you are spawning a constructor in C

[PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

Hello,
Has anyone got a chance to look into this patch?

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Iyer, Balaji V
 Sent: Tuesday, September 17, 2013 10:51 AM
 To: r...@redhat.com; Jason Merrill (ja...@redhat.com); 'Jeff Law'; Aldy
 Hernandez (al...@redhat.com)
 Cc: 'gcc-patches@gcc.gnu.org'
 Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 Hello,
   Has anyone had a chance to look at this. The C++ part is only a week
 old, but the C part has been in review for ~3 weeks. I would greatly 
 appreciate if
 someone could review this and approve for trunk if it is Ok for trunk.
 
 Thanks,
 
 Balaji V. Iyer.
 
  -Original Message-
  From: Iyer, Balaji V
  Sent: Wednesday, September 11, 2013 2:18 PM
  To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; Aldy
  Hernandez (al...@redhat.com)
  Cc: gcc-patches@gcc.gnu.org
  Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
  (and C++)
 
  Hello Everyone,
  Couple weeks back, I had submitted a patch for review that will
  implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into the C
  compiler. I recently finished C++ implementation also. In this email,
  I am attaching 2
  patches: 1 for C (and the common parts for C and C++) and 1 for C++.
  The C++ Changelog is labelled cp-ChangeLog.cilkplus and the other one
  is just ChangeLog.cilkplus. There isn't much changes in the C patch.
  Only noticeable changes would be moving functions to the common parts
  so that C++ can use them.
 
  It passes all the tests and does not affect  (by affect I mean fail a
  passing test or pass a failing one) any of the other tests in the testsuite
 directory.
 
  Is this Ok for trunk?
 
  Thanks,
 
  Balaji V. Iyer.
 
   -Original Message-
   From: Iyer, Balaji V
   Sent: Friday, August 30, 2013 1:02 PM
   To: gcc-patches@gcc.gnu.org
   Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for
   C
  
   The email seem to be bouncing gcc-patches. I have gzipped my patch.
  
   Thanks,
  
   Balaji V. Iyer.
  
  
 -Original Message-
 From: Iyer, Balaji V
 Sent: Friday, August 30, 2013 11:42 AM
 To: 'Aldy Hernandez'
 Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
 Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
 for C

 Hi Aldy,
   Attached, please find a fixed patch and the changelog entries.

  -Original Message-
  From: Aldy Hernandez [mailto:al...@redhat.com]
  Sent: Wednesday, August 28, 2013 2:36 PM
  To: Iyer, Balaji V
  Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
  Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and
  _Cilk_sync) for C
 
  On 08/27/13 16:27, Iyer, Balaji V wrote:
   Hello Aldy, I went through all the emails and here are the
   major issues that I could gather (other than lowering the
   keywords after gimplification, which I am skipping since it
   is more of an optimization for now).
 
  Ok, for now I am fine with delaying handling all this as a
  gimple tuple since most of your code lives in it's only little 
  world :).
  But I will go on record saying that part of the reason that
  you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such
  is because you don't
 have easy gimplified code to examine.
  Anyways, agreed, you can do this later.
 
  
   1. Calling the gimplify_cilk_spawn on top of the
   gimplify_expr before the switch-statement could slow the
   compiler down 2. I need a CILK_SPAWN_STMT case in the switch
   statement in gimplify_expr
   (). 3.
   No test for catching the suspicious spawned function warning 4.
   Reasoning for expanding the 2 builtin functions in
   builtins.c instead of just inserting the appropriate
   expanded-code when I am inserting the function call.
  
   Did I miss anything else (or misunderstand anything you pointed 
   out)?
  
   Here are my answers to those questions above and am
   attaching a fixed patch with the changelog entries:
  
   1  2(partial): There are 3 places where we could have 
   _Cilk_spawn:
   INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and
   MODIFY_EXPRS
 are
   both gimplified using gimplify_modify_expr. I have moved the
   cilk_detect_spawn into this function. We will go into the
   cilk_detect_spawn if cilk plus is enabled, and if there is a
   cilk_frame (meaning the function has a Cilk_spawn in it)
   thereby reducing the number of hits into this function 
   significantly.
   Inside this function, it will go into the function that has
   a spawned function call and then unwrap the CILK_SPAWN_STMT
   wrapper and returns true. This shouldn't cause a huge
   compilation time
   hit. 2.
   To handle CALL_EXPR (e.g. _Cilk_spawn foo (x), where

RE: [PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

Err...I hit the send button too quickly, thus making a stupid grammatical 
error.. it should say Did anyone get a chance to look into this patch? Sorry 
about this. 

-Balaji V. Iyer.

 -Original Message-
 From: Iyer, Balaji V
 Sent: Monday, September 23, 2013 7:14 PM
 To: 'r...@redhat.com'; 'Jason Merrill (ja...@redhat.com)'; 'Jeff Law'; 'Aldy
 Hernandez (al...@redhat.com)'
 Cc: 'gcc-patches@gcc.gnu.org'
 Subject: [PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 (and C++)
 
 Hello,
   Has anyone got a chance to look into this patch?
 
 Thanks,
 
 Balaji V. Iyer.
 
  -Original Message-
  From: Iyer, Balaji V
  Sent: Tuesday, September 17, 2013 10:51 AM
  To: r...@redhat.com; Jason Merrill (ja...@redhat.com); 'Jeff Law'; Aldy
  Hernandez (al...@redhat.com)
  Cc: 'gcc-patches@gcc.gnu.org'
  Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
  (and C++)
 
  Hello,
  Has anyone had a chance to look at this. The C++ part is only a week
  old, but the C part has been in review for ~3 weeks. I would greatly
  appreciate if someone could review this and approve for trunk if it is Ok 
  for
 trunk.
 
  Thanks,
 
  Balaji V. Iyer.
 
   -Original Message-
   From: Iyer, Balaji V
   Sent: Wednesday, September 11, 2013 2:18 PM
   To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; Aldy
   Hernandez (al...@redhat.com)
   Cc: gcc-patches@gcc.gnu.org
   Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for
   C (and C++)
  
   Hello Everyone,
 Couple weeks back, I had submitted a patch for review that will
   implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into the C
   compiler. I recently finished C++ implementation also. In this
   email, I am attaching 2
   patches: 1 for C (and the common parts for C and C++) and 1 for C++.
   The C++ Changelog is labelled cp-ChangeLog.cilkplus and the other
   one is just ChangeLog.cilkplus. There isn't much changes in the C patch.
   Only noticeable changes would be moving functions to the common
   parts so that C++ can use them.
  
 It passes all the tests and does not affect  (by affect I mean fail
   a passing test or pass a failing one) any of the other tests in the
   testsuite
  directory.
  
 Is this Ok for trunk?
  
   Thanks,
  
   Balaji V. Iyer.
  
-Original Message-
From: Iyer, Balaji V
Sent: Friday, August 30, 2013 1:02 PM
To: gcc-patches@gcc.gnu.org
Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
for C
   
The email seem to be bouncing gcc-patches. I have gzipped my patch.
   
Thanks,
   
Balaji V. Iyer.
   
   
  -Original Message-
  From: Iyer, Balaji V
  Sent: Friday, August 30, 2013 11:42 AM
  To: 'Aldy Hernandez'
  Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
  Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and
  _Cilk_sync) for C
 
  Hi Aldy,
  Attached, please find a fixed patch and the changelog entries.
 
   -Original Message-
   From: Aldy Hernandez [mailto:al...@redhat.com]
   Sent: Wednesday, August 28, 2013 2:36 PM
   To: Iyer, Balaji V
   Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
   Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and
   _Cilk_sync) for C
  
   On 08/27/13 16:27, Iyer, Balaji V wrote:
Hello Aldy, I went through all the emails and here are the
major issues that I could gather (other than lowering the
keywords after gimplification, which I am skipping since
it is more of an optimization for now).
  
   Ok, for now I am fine with delaying handling all this as a
   gimple tuple since most of your code lives in it's only little 
   world :).
   But I will go on record saying that part of the reason that
   you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and
   such is because you don't
  have easy gimplified code to examine.
   Anyways, agreed, you can do this later.
  
   
1. Calling the gimplify_cilk_spawn on top of the
gimplify_expr before the switch-statement could slow the
compiler down 2. I need a CILK_SPAWN_STMT case in the
switch statement in gimplify_expr
(). 3.
No test for catching the suspicious spawned function warning 4.
Reasoning for expanding the 2 builtin functions in
builtins.c instead of just inserting the appropriate
expanded-code when I am inserting the function call.
   
Did I miss anything else (or misunderstand anything you pointed
 out)?
   
Here are my answers to those questions above and am
attaching a fixed patch with the changelog entries:
   
1  2(partial): There are 3 places where we could have 
_Cilk_spawn:
INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and
MODIFY_EXPRS
  are
both gimplified using

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Iyer, Balaji V]

Hello,
Has anyone had a chance to look at this. The C++ part is only a week 
old, but the C part has been in review for ~3 weeks. I would greatly appreciate 
if someone could review this and approve for trunk if it is Ok for trunk.

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Iyer, Balaji V
 Sent: Wednesday, September 11, 2013 2:18 PM
 To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; Aldy
 Hernandez (al...@redhat.com)
 Cc: gcc-patches@gcc.gnu.org
 Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and 
 C++)
 
 Hello Everyone,
   Couple weeks back, I had submitted a patch for review that will
 implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into the C compiler. I
 recently finished C++ implementation also. In this email, I am attaching 2
 patches: 1 for C (and the common parts for C and C++) and 1 for C++. The C++
 Changelog is labelled cp-ChangeLog.cilkplus and the other one is just
 ChangeLog.cilkplus. There isn't much changes in the C patch. Only noticeable
 changes would be moving functions to the common parts so that C++ can use
 them.
 
   It passes all the tests and does not affect  (by affect I mean fail a 
 passing
 test or pass a failing one) any of the other tests in the testsuite directory.
 
   Is this Ok for trunk?
 
 Thanks,
 
 Balaji V. Iyer.
 
  -Original Message-
  From: Iyer, Balaji V
  Sent: Friday, August 30, 2013 1:02 PM
  To: gcc-patches@gcc.gnu.org
  Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
  The email seem to be bouncing gcc-patches. I have gzipped my patch.
 
  Thanks,
 
  Balaji V. Iyer.
 
 
-Original Message-
From: Iyer, Balaji V
Sent: Friday, August 30, 2013 11:42 AM
To: 'Aldy Hernandez'
Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
for C
   
Hi Aldy,
Attached, please find a fixed patch and the changelog entries.
   
 -Original Message-
 From: Aldy Hernandez [mailto:al...@redhat.com]
 Sent: Wednesday, August 28, 2013 2:36 PM
 To: Iyer, Balaji V
 Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync)
 for C

 On 08/27/13 16:27, Iyer, Balaji V wrote:
  Hello Aldy, I went through all the emails and here are the
  major issues that I could gather (other than lowering the
  keywords after gimplification, which I am skipping since it is
  more of an optimization for now).

 Ok, for now I am fine with delaying handling all this as a
 gimple tuple since most of your code lives in it's only little world 
 :).
 But I will go on record saying that part of the reason that you
 have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is
 because you don't
have easy gimplified code to examine.
 Anyways, agreed, you can do this later.

 
  1. Calling the gimplify_cilk_spawn on top of the gimplify_expr
  before the switch-statement could slow the compiler down 2. I
  need a CILK_SPAWN_STMT case in the switch statement in
  gimplify_expr
  (). 3.
  No test for catching the suspicious spawned function warning 4.
  Reasoning for expanding the 2 builtin functions in builtins.c
  instead of just inserting the appropriate expanded-code when I
  am inserting the function call.
 
  Did I miss anything else (or misunderstand anything you pointed 
  out)?
 
  Here are my answers to those questions above and am attaching
  a fixed patch with the changelog entries:
 
  1  2(partial): There are 3 places where we could have _Cilk_spawn:
  INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and
  MODIFY_EXPRS
are
  both gimplified using gimplify_modify_expr. I have moved the
  cilk_detect_spawn into this function. We will go into the
  cilk_detect_spawn if cilk plus is enabled, and if there is a
  cilk_frame (meaning the function has a Cilk_spawn in it)
  thereby reducing the number of hits into this function 
  significantly.
  Inside this function, it will go into the function that has a
  spawned function call and then unwrap the CILK_SPAWN_STMT
  wrapper and returns true. This shouldn't cause a huge
  compilation time
  hit. 2.
  To handle CALL_EXPR (e.g. _Cilk_spawn foo (x), where foo
  returns a void or the return value of it is ignored), I have
  added a CILK_SPAWN_STMT
case.
  Again, I am calling the detect_cilk_spawn and we will only
  step into this function if Cilk Plus is enabled and if there
  is a cilk-frame (i.e saying the function has a cilk spawn in
  it). If there is an error (seen_error () == true), then it
  just falls through into CALL_EXPR and is handled like a normal
  call expr not spawned expression. 3. This warning rarely get

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

[Jeff Law]

On 09/17/2013 08:50 AM, Iyer, Balaji V wrote:

Hello, Has anyone had a chance to look at this. The C++ part is only
a week old, but the C part has been in review for ~3 weeks. I would
greatly appreciate if someone could review this and approve for trunk
if it is Ok for trunk.

Obviously not yet.  Everyone is pretty busy right now.

jeff

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

+   case CILK_SYNC_STMT: +{ +   if (!cfun-cilk_frame_decl) +
{ + error_at (input_location, expected %_Cilk_spawn% before
 +  %_Cilk_sync%); +  ret = GS_ERROR; +
 }


First, surely you have a location you can use, instead of the
generic input_location (perhaps the location for the
CILK_SYNC_STMT??).  Also, Can you not check for this in
c_finish_cilk_sync_stmt(), or the corresponding code-- that is, in
the FE somewhere?  And hopefully, in a place you can share with the
C++ FE?  If it is a real pain, I am willing to let this go, since
it happens only in the Cilk code path, though the general trend
(especially with Andrew's proposed changes) is to do all type
checking as close to the source as possible.


If you look at the codes above (e.g. TRUTH_XOR_EXPR), they all use
input_location. Also, in the beginning of the function there is a
line  like this:

if (save_expr != error_mark_node  EXPR_HAS_LOCATION (*expr_p))
input_location = EXPR_LOCATION (*expr_p);


Yes, that is old legacy code.  The present move is to steer away from 
input_location altogether.



For the 2nd point, there can be a case where (with the help of Gotos)
_Cilk_sync can come before _Cilk_spawn. So, the only way to do this
check is to do it after the entire function is parsed.


Fair enough.

Alright, I'm fine with this current incantation.  Thanks for all your 
hard work taking care of the things I've pointed out.


It's now up to one of the core maintainers to take it from here.

Aldy

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

On 08/27/13 16:27, Iyer, Balaji V wrote:

Hello Aldy, I went through all the emails and here are the major
issues that I could gather (other than lowering the keywords after
gimplification, which I am skipping since it is more of an
optimization for now).


Ok, for now I am fine with delaying handling all this as a gimple tuple 
since most of your code lives in it's only little world :).  But I will 
go on record saying that part of the reason that you have to handle 
CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is because you don't have 
easy gimplified code to examine.  Anyways, agreed, you can do this later.




1. Calling the gimplify_cilk_spawn on top of the gimplify_expr before
the switch-statement could slow the compiler down 2. I need a
CILK_SPAWN_STMT case in the switch statement in gimplify_expr (). 3.
No test for catching the suspicious spawned function warning 4.
Reasoning for expanding the 2 builtin functions in builtins.c instead
of just inserting the appropriate expanded-code when I am inserting
the function call.

Did I miss anything else (or misunderstand anything you pointed
out)?

Here are my answers to those questions above and am attaching a fixed
patch with the changelog entries:

1  2(partial): There are 3 places where we could have _Cilk_spawn:
INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and MODIFY_EXPRS are
both gimplified using gimplify_modify_expr. I have moved the
cilk_detect_spawn into this function. We will go into the
cilk_detect_spawn if cilk plus is enabled, and if there is a
cilk_frame (meaning the function has a Cilk_spawn in it) thereby
reducing the number of hits into this function significantly. Inside
this function, it will go into the function that has a spawned
function call and then unwrap the CILK_SPAWN_STMT wrapper and returns
true. This shouldn't cause a huge compilation time hit. 2. To handle
CALL_EXPR (e.g. _Cilk_spawn foo (x), where foo returns a void or the
return value of it is ignored), I have added a CILK_SPAWN_STMT case.
Again, I am calling the detect_cilk_spawn and we will only step into
this function if Cilk Plus is enabled and if there is a cilk-frame
(i.e saying the function has a cilk spawn in it). If there is an
error (seen_error () == true), then it just falls through into
CALL_EXPR and is handled like a normal call expr not spawned
expression. 3. This warning rarely get hit, and I have seen it hit


See my comments below on this.


only when you are spawning a constructor in C++. To my knowledge, we
have not had anyone report that they have hit this warning. I just


Ok, leave the warning, but do include a test case.


kept it in there just in case as a heads-up. 4. The reason why I am
handling pop_frame and detach functions in builtins.c is because one
of the things that LTO does is to remove the frame pointer. All Cilk
functions must use the frame pointer. When LTO is invoked, it is hard
to see what function is a cilk function and what is not. The CFUN
flag that says it is a cilk function gets cleared out. But, the
builtin functions are expanded during LTO phase and I set the
is_cilk_function flag when it is expanding pop_frame and detach. The
other option that I was thinking was to have frame pointer on when
Cilk Plus is enabled, but this is a bit over-kill and can cause
performance issues.


I think the reason you have to do all these gymnastics is bececause you 
are waiting so late to expand.  You could expand into trees and not have 
to worry about frame pointers and such.  See fold_builtin_* in 
builtins.c.  *However*, if you think you can generate better code by 
delaying expansion all the way until RTL, then I'm fine with your 
current approach.




Also, I had added a couple more tests to catch a couple cases.



+  /* Implicit _Cilk_sync must be inserted right before any return statement
+ if there is a _Cilk_spawn in the function (checked by seeing if
+ cilk_frame_decl is not NULL_TREE).  If the user has provided a
+ _Cilk_sync, the optimizer should remove this duplicate one.  */
+  if (flag_enable_cilkplus  cfun-cilk_frame_decl != NULL_TREE)


Again, never document the obvious things your code is doing.  For 
example, you can remove (checked by seeing if
 + cilk_frame_decl is not NULL_TREE).  It's obvious by looking at 
the code.



+  /* If there are errors, there is no point in expanding the
+ _Cilk_spawn.  Just gimplify like a normal CALL_EXPR.  */
+   !seen_error ())


Similarly here.  No need to document the obvious.


+  /* If there are errors, there is no point in expanding the
+ _Cilk_spawn.  Just gimplify like a normal MODIFY or INIT_EXPR.  */
+   !seen_error ())


And here.

Alos, if the canonical way of checking if a function has a cilk_spawn in 
it is always cfun-cilk_frame_decl != NULL_TREE, then you should 
abstract this into an inline function.  The implementation will be 
trivial to change if we ever decide to keep that information elsewhere. 
 Perhaps something 

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

On 08/21/13 14:59, Iyer, Balaji V wrote:




-Original Message- From: Aldy Hernandez
[mailto:al...@redhat.com] Sent: Wednesday, August 21, 2013 11:31 AM
To: Iyer, Balaji V Cc: r...@redhat.com; Jeff Law;
gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Cilk Keywords
(_Cilk_spawn and _Cilk_sync) for C

Even more review stuff.  Are you keeping track of all this Balaji?
:)



Yes I am. Please keep an eye out for a fixed patch soon.


+  if (warn) +warning (0, suspicious use of _Cilk_spawn);


First, as I've mentioned, this error message is very ambiguous. You
should strive to provide better error messages.  See my previous
comment on this same line of code.

However... for all the checking you do in cilk_valid_spawn, I
don't see a single corresponding test.



Well, the name of the function is misleading.  I will fix that. I
think it should call it detect_cilk_spawn instead

What the function does it NOT to find whether there are syntax or
other issues in the spawned statement, but to check if spawn is used
in appropriate location. Here are some cases were you can use spawn
(I am sure I am missing something):

X = _Cilk_spawn foo ();

_Cilk_spawn foo ()

operator=(x, _Cilk_spawn foo ())

and these things can be kept in different kind of trees and so
adding this in individual tree's case statement can be a lot of
code-addition and is error prone.


It still sounds like some sort of type checking best done at the source
level.  You can analyze all this in the parser as suggested.  Checking 
if spawn is used in the appropriate location, as you mention, should not 
be done in the gimplifier.


And btw, the reason you have to check all these variants (whether in a a 
MODIFY_EXPR, INIT_EXPR, CALL_EXPR, operator, etc) is because you are 
dealing with trees.  If you handled this as a gimple tuple, things would 
have already been simplified enough for you to only have to worry about 
GIMPLE_CALL.  That being said, I understand it would be more work to 
redesign things to work at the gimple level, but it is something we want 
to do eventually.




The warning you see is more like an heads up. I can take out of if
you like. If you notice, when I see an error, I don't bother
gimplifying the spawned function (but just let the compiler go ahead
as a regular function call) thereby not creating a new nested
function etc.


No, I don't want you to take it out.  For that matter (as I've suggested 
earlier up-thread), I would like you to expand on this and provide more 
verbose errors/warnings for each of the different things you can catch, 
instead of the the generic suspicious warning.





May I stress again the importance of tests-- which are especially
critical for new language features.  You don't want cilk silently
breaking thus rendering all your hard work moot, do you? :))

You particularly need tests for all quirks described in the Cilk
Plus language specification around here:

A program is considered ill formed if the _Cilk_spawn form of
this expression appears other than in one of the following
contexts: [blah blah blah].



I have several of those already (e.g. using spawn outside a
function, spawning something that is not a function, etc)


I just didn't see any test for the suspicious warning.






+  /* Strip off any conversion to void.  It does not affect
whether spawn + is supported here.  */ +  if (TREE_CODE
(exp) == CONVERT_EXPR  VOID_TYPE_P (TREE_TYPE

(exp)))

+exp = TREE_OPERAND (exp, 0);


Don't you need to strip off various levels here with a loop?
Also, could any of the following do the job? STRIP_NOPS,
STRIP_TYPE_NOPS, STRIP_USELESS_TYPE_CONVERSION.


@@ -7086,6 +7087,19 @@ gimplify_expr (tree *expr_p, gimple_seq
*pre_p,

gimple_seq *post_p,

else if (ret != GS_UNHANDLED) break;

+  if (flag_enable_cilkplus 
lang_hooks.cilkplus.cilk_valid_spawn (expr_p)) +{ +   /* If
there are errors, there is no point in expanding the +
_Cilk_spawn.  Just gimplify like a normal call expr.  */ +if
(!seen_error ()) +  { +   ret = (enum gimplify_status) +
lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,

post_p);

+ if (ret != GS_UNHANDLED) +continue; + } + 
} +


Oh, hell no!  You do realize you are drilling down and walking
every single expression being passed to the gimplifier to find
your spawn? That's not cool.  You need to find some way to
annotate expressions or do this more efficiently.  It may help to
bootstrap with -fcilkplus and do performance analysis, to make sure
you're not making the compiler slower on the non cilkplus code
path.

Could you not let the gimplifier do its thing and add a case for
CILK_SPAWN_STMT where you do the unwrapping and everything else?
I do realize that cilk_valid_spawn() is doing all sorts of type
checking, and validation, but the gimplifier is really not the
place to do this.  When possible, you should do type checking as
close to the source as possible, thus-- at the parser.  See how

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

Even more review stuff.  Are you keeping track of all this Balaji? :)


+  if (warn)
+warning (0, suspicious use of _Cilk_spawn);


First, as I've mentioned, this error message is very ambiguous.  You 
should strive to provide better error messages.  See my previous comment 
on this same line of code.


However... for all the checking you do in cilk_valid_spawn, I don't see 
a single corresponding test.


May I stress again the importance of tests-- which are especially
critical for new language features.  You don't want cilk silently
breaking thus rendering all your hard work moot, do you? :))

You particularly need tests for all quirks described in the Cilk Plus
language specification around here:

A program is considered ill formed if the _Cilk_spawn form of this
expression appears other than in one of the following contexts: [blah
blah blah].



+  /* Strip off any conversion to void.  It does not affect whether spawn
+ is supported here.  */
+  if (TREE_CODE (exp) == CONVERT_EXPR  VOID_TYPE_P (TREE_TYPE (exp)))
+exp = TREE_OPERAND (exp, 0);


Don't you need to strip off various levels here with a loop?  Also, 
could any of the following do the job? STRIP_NOPS, STRIP_TYPE_NOPS, 
STRIP_USELESS_TYPE_CONVERSION.



@@ -7086,6 +7087,19 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, 
gimple_seq *post_p,
   else if (ret != GS_UNHANDLED)
break;

+  if (flag_enable_cilkplus  lang_hooks.cilkplus.cilk_valid_spawn 
(expr_p))
+   {
+ /* If there are errors, there is no point in expanding the
+_Cilk_spawn.  Just gimplify like a normal call expr.  */
+ if (!seen_error ())
+   {
+ ret = (enum gimplify_status)
+   lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p, post_p);
+ if (ret != GS_UNHANDLED)
+   continue;
+   }
+   }
+


Oh, hell no!  You do realize you are drilling down and walking every 
single expression being passed to the gimplifier to find your spawn? 
That's not cool.  You need to find some way to annotate expressions or 
do this more efficiently.  It may help to bootstrap with -fcilkplus and 
do performance analysis, to make sure you're not making the compiler 
slower on the non cilkplus code path.


Could you not let the gimplifier do its thing and add a case for 
CILK_SPAWN_STMT where you do the unwrapping and everything else?  I do 
realize that cilk_valid_spawn() is doing all sorts of type checking, and 
validation, but the gimplifier is really not the place to do this.  When 
possible, you should do type checking as close to the source as 
possible, thus-- at the parser.  See how c_finish_omp_for() is called 
from the FE to do type checking, build the OMP_FOR tree node, *and* do 
the add_stmt().  Perhaps you need corresponding a 
c_finish_cilk_{spawn,sync}.  Definitely worth looking into.  But I can 
tell you now, drilling down into every expression being gimplified is a 
no-go.


Also, do you realy need two hooks to recognize spawns: recognize_spawn 
and cilk_valid_spawn?  And are C/C++ so different that you need a hook

with different versions of each?


+/* Returns a setjmp CALL_EXPR with FRAME-context as its parameter.  */
+
+tree
+cilk_call_setjmp (tree frame)


Is this used anywhere else but in this file?  If not, please declare static.


+/* Expands the __cilkrts_pop_frame function call stored in EXP.
+   Returns const0_rtx.  */
+
+void
+expand_builtin_cilk_pop_frame (tree exp)

[snip]

+/* Expands the cilk_detach function call stored in EXP.  Returns const0_rtx.  
*/
+
+void
+expand_builtin_cilk_detach (tree exp)


Do these builtins really have to be expanded into rtl?  Can this not be 
modeled with trees or gimple?  Expansion into rtl should be used for 
truly architecture dependent stuff that cannot be modeled with anything 
higher level.


For the memory barrier stuff, we already have Andrew's atomic and memory 
model infrastructure which I think should be enough to model whatever 
you are expanding into RTL here.  But I may be wrong...


Aldy

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Jeff Law]

On 08/21/2013 09:31 AM, Aldy Hernandez wrote:


May I stress again the importance of tests-- which are especially
critical for new language features.  You don't want cilk silently
breaking thus rendering all your hard work moot, do you? :))
Agreed.  While we don't have a strict policy for testing new features, 
adding tests for this kind of stuff is highly encouraged.  Not everyone 
doing GCC development is going to be familiar enough with Cilk+ and how 
their patch might interact with the Cilk+ support.


Having tests in the testsuite makes it much less likely someone will 
break the Cilk+ support accidentally.



+  if (flag_enable_cilkplus 
lang_hooks.cilkplus.cilk_valid_spawn (expr_p))
+{
+  /* If there are errors, there is no point in expanding the
+ _Cilk_spawn.  Just gimplify like a normal call expr.  */
+  if (!seen_error ())
+{
+  ret = (enum gimplify_status)
+lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p, post_p);
+  if (ret != GS_UNHANDLED)
+continue;
+}
+}
+


Oh, hell no!  You do realize you are drilling down and walking every
single expression being passed to the gimplifier to find your spawn?
That's not cool.  You need to find some way to annotate expressions or
do this more efficiently.  It may help to bootstrap with -fcilkplus and
do performance analysis, to make sure you're not making the compiler
slower on the non cilkplus code path.
Yea, that would definitely be a problem (walking every expression 
looking for cilk spawns inside).  Having gimple nodes for these things 
would seem to make sense to me as well.




Could you not let the gimplifier do its thing and add a case for
CILK_SPAWN_STMT where you do the unwrapping and everything else?  I do
realize that cilk_valid_spawn() is doing all sorts of type checking, and
validation, but the gimplifier is really not the place to do this.  When
possible, you should do type checking as close to the source as
possible, thus-- at the parser.  See how c_finish_omp_for() is called
from the FE to do type checking, build the OMP_FOR tree node, *and* do
the add_stmt().  Perhaps you need corresponding a
c_finish_cilk_{spawn,sync}.  Definitely worth looking into.  But I can
tell you now, drilling down into every expression being gimplified is a
no-go.
Yea, we *really* want the gimplification  checking separated.   If we 
think about where Andrew's proposals take us, then the checking needs to 
move into the front-end.  gimplification should be relegated, to the 
fullest extent possible, to converting the IL down to gimple.





Jeff

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Iyer, Balaji V]

 -Original Message-
 From: Aldy Hernandez [mailto:al...@redhat.com]
 Sent: Wednesday, August 21, 2013 11:31 AM
 To: Iyer, Balaji V
 Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
 Even more review stuff.  Are you keeping track of all this Balaji? :)
 

Yes I am. Please keep an eye out for a fixed patch soon.

  +  if (warn)
  +warning (0, suspicious use of _Cilk_spawn);
 
 First, as I've mentioned, this error message is very ambiguous.  You should 
 strive
 to provide better error messages.  See my previous comment on this same line
 of code.
 
 However... for all the checking you do in cilk_valid_spawn, I don't see a 
 single
 corresponding test.
 

Well, the name of the function is misleading.  I will fix that. I think it 
should call it detect_cilk_spawn instead

What the function does it NOT to find whether there are syntax or other issues 
in the spawned statement, but to check if spawn is used in appropriate location.
Here are some cases were you can use spawn (I am sure I am missing something):

X = _Cilk_spawn foo ();

_Cilk_spawn foo ()

operator=(x, _Cilk_spawn foo ())

and these things can be kept in different kind of trees and so adding this in 
individual tree's case statement can be a lot of code-addition and is error 
prone.

The warning you see is more like an heads up. I can take out of if you like. 
If you notice, when I see an error, I don't bother gimplifying the spawned 
function (but just let the compiler go ahead as a regular function call) 
thereby not creating a new nested function etc.

 May I stress again the importance of tests-- which are especially critical 
 for new
 language features.  You don't want cilk silently breaking thus rendering all 
 your
 hard work moot, do you? :))
 
 You particularly need tests for all quirks described in the Cilk Plus language
 specification around here:
 
 A program is considered ill formed if the _Cilk_spawn form of this expression
 appears other than in one of the following contexts: [blah blah blah].


I have several of those already (e.g. using spawn outside a function, spawning 
something that is not a function, etc)
 
 
  +  /* Strip off any conversion to void.  It does not affect whether spawn
  + is supported here.  */
  +  if (TREE_CODE (exp) == CONVERT_EXPR  VOID_TYPE_P (TREE_TYPE
 (exp)))
  +exp = TREE_OPERAND (exp, 0);
 
 Don't you need to strip off various levels here with a loop?  Also, could any 
 of
 the following do the job? STRIP_NOPS, STRIP_TYPE_NOPS,
 STRIP_USELESS_TYPE_CONVERSION.
 
  @@ -7086,6 +7087,19 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p,
 gimple_seq *post_p,
 else if (ret != GS_UNHANDLED)
  break;
 
  +  if (flag_enable_cilkplus  lang_hooks.cilkplus.cilk_valid_spawn 
  (expr_p))
  +   {
  + /* If there are errors, there is no point in expanding the
  +_Cilk_spawn.  Just gimplify like a normal call expr.  */
  + if (!seen_error ())
  +   {
  + ret = (enum gimplify_status)
  +   lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
 post_p);
  + if (ret != GS_UNHANDLED)
  +   continue;
  +   }
  +   }
  +
 
 Oh, hell no!  You do realize you are drilling down and walking every single
 expression being passed to the gimplifier to find your spawn?
 That's not cool.  You need to find some way to annotate expressions or do this
 more efficiently.  It may help to bootstrap with -fcilkplus and do performance
 analysis, to make sure you're not making the compiler slower on the non 
 cilkplus
 code path.
 
 Could you not let the gimplifier do its thing and add a case for
 CILK_SPAWN_STMT where you do the unwrapping and everything else?  I do
 realize that cilk_valid_spawn() is doing all sorts of type checking, and 
 validation,
 but the gimplifier is really not the place to do this.  When possible, you 
 should do
 type checking as close to the source as possible, thus-- at the parser.  See 
 how
 c_finish_omp_for() is called from the FE to do type checking, build the 
 OMP_FOR
 tree node, *and* do the add_stmt().  Perhaps you need corresponding a
 c_finish_cilk_{spawn,sync}.  Definitely worth looking into.  But I can tell 
 you
 now, drilling down into every expression being gimplified is a no-go.
 

Well, I think the name of the function is what that is misleading here.

I am not recursing through the entire tree to find the spawn keyword here. What 
I am trying to see is if *expr_p is an INIT_EXPR, or TARGET_EXPR or a 
CALL_EXPR etc.

I do agree with you about one thing. I should first check to see if the 
function has a _Cilk_spawn before I go through and check for individual trees. 
That can be done easily by looking at cfun-cilk_frame_decl != NULL_TREE. That 
change I will make in the next patch. 

 Also, do you realy need two hooks to recognize spawns: recognize_spawn and
 cilk_valid_spawn?  And are C/C++ so different that you need

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

[rth, law, jakub: Your input required throughout...please.]

More review stuff...

Overall, I must say, I'm not a big fan of the super early expansion 
you're doing right after parsing.  I mean, you leave CILK_SPAWN and 
CILK_SYNC keywords as is (in tree form until gimplification) but there's 
this awful lot of expansion that goes on parallel to that.  For 
instance, for this code:


extern void testing();
extern void ending();

foo(){
_Cilk_spawn bar();
testing();
_Cilk_sync;
ending();
}

...right after parsing you already generate:

{
  __cilkrts_enter_frame_1 (D.1776);
  try
{
  _Cilk_spawn bar ();   // keywords as trees, fine
  testing ();
  _Cilk_sync;;  // keywords as trees, fine
  ending ();
}
  finally   // but all this try/finally
// support stuff too early??
{
  D.1776.pedigree = D.1776.worker-pedigree;
  if ((D.1776.flags  2) != 0)
{
  __cilkrts_save_fp_ctrl_state (D.1776);
  if (__builtin_setjmp (D.1776.ctx) == 0)
{
  __cilkrts_sync (D.1776);
}
  else
{
  if ((D.1776.flags  16) != 0)
{
  __cilkrts_rethrow (D.1776);
}
}
}
  D.1776.worker-pedigree.rank = D.1776.worker-pedigree.rank + 1;
  D.1776.worker-current_stack_frame = D.1776.call_parent;
  __cilkrts_pop_frame (D.1776);
  if (D.1776.flags != 16777216)
{
  __cilkrts_leave_frame (D.1776);
}
}
}

You seem to be hijacking finish_function(), to generate all this 
try/finally and builtin stuff here:



diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index f7ae648..ffd62c6 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -8380,6 +8380,12 @@ finish_function (void)
   /* Tie off the statement tree for this function.  */
   DECL_SAVED_TREE (fndecl) = pop_stmt_list (DECL_SAVED_TREE (fndecl));

+  /* IF the function has _Cilk_spawn in front of a function call inside it
+ i.e. it is a spawning function, then add the appropriate Cilk plus
+ functions inside.  */
+  if (flag_enable_cilkplus  cfun-calls_cilk_spawn == 1)
+cfun-cilk_frame_decl = insert_cilk_frame (fndecl);


I would've preferred to leave all expansion until *at least* 
gimplification.  Although, looking at how OMP works, expansion happens 
even further down after the CFG has been built and EH has been handled. 
 Seeing that _Cilk_spawn and _Cilk_sync heavily alter the flow (and 
possibly exceptions) of the program, I would guess that you need to 
follow a similar path with these two Cilk keywords.


I don't think this is a blocker, especially since most everything seems 
to be contained in Cilk specific files, but I would like to get some 
feedback from Jeff/Richard/Jakub, and/or some other more globally savvy 
people :).  Perhaps this could be done as a follow-up patch, if 
necessary.  Having said that, at least expansion in finish_function() 
feels weird.



+tree
+c_build_cilk_spawn (location_t loc, tree call)
+{
+  if (!cilkplus_set_spawn_marker (loc, call))
+return error_mark_node;
+  tree spawn_stmt = build1 (CILK_SPAWN_STMT, TREE_TYPE (call), call);


Do you need a TREE_TYPE here at all?  Is it used anywhere?


+/* Marks CALL, a CALL_EXPR, as a spawned function call.  */
+
+tree
+c_build_cilk_spawn (location_t loc, tree call)
+{
+  if (!cilkplus_set_spawn_marker (loc, call))
+return error_mark_node;


Can you inline cilkplus_set_spawn_marker?  It's not used anywhere else 
and it's relatively small.  If you need it for C++, perhaps you can make 
c_build_cilk_spawn() generic enough to be used for both FE's?



+/* Helper function for walk_tree.  If *TP is a CILK_SPAWN_STMT, then unwrap
+   this wrapper and  *WALK_SUBTREES is set to 0.  The function returns
+   NULL_TREE regardless.  */
+
+static tree
+unwrap_cilk_sync_stmt (tree *tp, int *walk_subtrees, void *)
+{
+  if (TREE_CODE (*tp) == CILK_SPAWN_STMT)
+{
+  *tp = CILK_SPAWN_FN (*tp);
+  *walk_subtrees = 0;
+}
+  return NULL_TREE;
+}


Why is this called unwrap_cilk_sync_stmt when it is unwrapping a spawn?


+case CILK_SYNC_STMT:
+  pp_string (buffer, _Cilk_sync;);
+  break;


Drop the semi-colon.  The end of dump_generic_node() should add the 
semi-colon if it's a statement.  I bet you're probably getting two 
semi-colons at the end of _Cilk_sync in the dump.



+/* Returns a wrapper function for a _Cilk_spawn.  */
+
+static tree
+build_cilk_wrapper (tree exp, tree *args_out)


This name is confusing.  The world build is usually used to denote 
front-end building of trees, not gimplification.  Seeing that 
build_cilk_wrapper() is only called from gimplify_cilk_spawn(), it's a 
bit confusing.



+/* This function will expand a cilk_sync call.  */
+
+static tree
+build_cilk_sync (void)
+{
+  tree frame = cfun-cilk_frame_decl;


Similarly with this as well, which is 

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Iyer, Balaji V]

HI Aldy et al.,
I would like to address the early expansion question. The reason why I 
did it is this way is because it was straight-forward for me to implement. I 
did some preliminary analysis, and it wasn't blocking any major optimization. 
Also, after parsing all I am really doing is to insert a couple functions. The 
expansion is actually done during gimplification (in gimplify_expr).

If it is Ok with everyone, I would like to keep it as-is for now. After 
I finish getting all the Cilk Plus parts into trunk, I will go back and look 
into optimizing this. This way, I can have some time (november to feb, during 
stage3) to do some more analysis and see if I can come up with a better 
solution.

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Aldy Hernandez [mailto:al...@redhat.com]
 Sent: Tuesday, August 20, 2013 6:04 PM
 To: Iyer, Balaji V
 Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org; Jakub Jelinek
 Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
 [rth, law, jakub: Your input required throughout...please.]
 
 More review stuff...
 
 Overall, I must say, I'm not a big fan of the super early expansion you're 
 doing
 right after parsing.  I mean, you leave CILK_SPAWN and CILK_SYNC keywords as
 is (in tree form until gimplification) but there's this awful lot of 
 expansion that
 goes on parallel to that.  For instance, for this code:
 
 extern void testing();
 extern void ending();
 
 foo(){
  _Cilk_spawn bar();
  testing();
  _Cilk_sync;
  ending();
 }
 
 ...right after parsing you already generate:
 
 {
__cilkrts_enter_frame_1 (D.1776);
try
  {
_Cilk_spawn bar ();// keywords as trees, fine
testing ();
_Cilk_sync;;   // keywords as trees, fine
ending ();
  }
finally// but all this try/finally
   // support stuff too early??
  {
D.1776.pedigree = D.1776.worker-pedigree;
if ((D.1776.flags  2) != 0)
  {
__cilkrts_save_fp_ctrl_state (D.1776);
if (__builtin_setjmp (D.1776.ctx) == 0)
  {
__cilkrts_sync (D.1776);
  }
else
  {
if ((D.1776.flags  16) != 0)
  {
__cilkrts_rethrow (D.1776);
  }
  }
  }
D.1776.worker-pedigree.rank = D.1776.worker-pedigree.rank + 1;
D.1776.worker-current_stack_frame = D.1776.call_parent;
__cilkrts_pop_frame (D.1776);
if (D.1776.flags != 16777216)
  {
__cilkrts_leave_frame (D.1776);
  }
  }
 }
 
 You seem to be hijacking finish_function(), to generate all this try/finally 
 and
 builtin stuff here:
 
  diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index f7ae648..ffd62c6
  100644
  --- a/gcc/c/c-decl.c
  +++ b/gcc/c/c-decl.c
  @@ -8380,6 +8380,12 @@ finish_function (void)
 /* Tie off the statement tree for this function.  */
 DECL_SAVED_TREE (fndecl) = pop_stmt_list (DECL_SAVED_TREE
  (fndecl));
 
  +  /* IF the function has _Cilk_spawn in front of a function call inside it
  + i.e. it is a spawning function, then add the appropriate Cilk plus
  + functions inside.  */
  +  if (flag_enable_cilkplus  cfun-calls_cilk_spawn == 1)
  +cfun-cilk_frame_decl = insert_cilk_frame (fndecl);
 
 I would've preferred to leave all expansion until *at least* gimplification.
 Although, looking at how OMP works, expansion happens even further down
 after the CFG has been built and EH has been handled.
   Seeing that _Cilk_spawn and _Cilk_sync heavily alter the flow (and possibly
 exceptions) of the program, I would guess that you need to follow a similar 
 path
 with these two Cilk keywords.
 
 I don't think this is a blocker, especially since most everything seems to be
 contained in Cilk specific files, but I would like to get some feedback from
 Jeff/Richard/Jakub, and/or some other more globally savvy people :).  Perhaps
 this could be done as a follow-up patch, if necessary.  Having said that, at 
 least
 expansion in finish_function() feels weird.
 
  +tree
  +c_build_cilk_spawn (location_t loc, tree call) {
  +  if (!cilkplus_set_spawn_marker (loc, call))
  +return error_mark_node;
  +  tree spawn_stmt = build1 (CILK_SPAWN_STMT, TREE_TYPE (call), call);
 
 Do you need a TREE_TYPE here at all?  Is it used anywhere?
 
  +/* Marks CALL, a CALL_EXPR, as a spawned function call.  */
  +
  +tree
  +c_build_cilk_spawn (location_t loc, tree call) {
  +  if (!cilkplus_set_spawn_marker (loc, call))
  +return error_mark_node;
 
 Can you inline cilkplus_set_spawn_marker?  It's not used anywhere else and 
 it's
 relatively small.  If you need it for C++, perhaps you can make
 c_build_cilk_spawn() generic enough to be used for both FE's?
 
  +/* Helper function for walk_tree.  If *TP is a CILK_SPAWN_STMT, then
 unwrap

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

@@ -960,6 +960,7 @@ SCEV_H = tree-scalar-evolution.h $(GGC_H) tree-chrec.h 
$(PARAMS_H)
 OMEGA_H = omega.h $(PARAMS_H)
 TREE_DATA_REF_H = tree-data-ref.h $(OMEGA_H) graphds.h $(SCEV_H)
 TREE_INLINE_H = tree-inline.h
+CILK_H = cilk.h
 REAL_H = real.h $(MACHMODE_H)
 IRA_INT_H = ira.h ira-int.h $(CFGLOOP_H) alloc-pool.h


Doesn't cilk.h depend on tree.h?  So shouldn't that be:

CILK_H = cilk.h $(TREE_H)

Which would mean that whoever includes cilk.h doesn't need to include 
tree.h.  Although... what I would prefer is not to include tree.h from 
cilk.h at all, and have the caller/includer of cilk.h include tree.h first.



+c-family/cilk.o : c-family/cilk.c $(TREE_H) $(SYSTEM_H) $(CONFIG_H) toplev.h \
+   $(TREE_H) coretypes.h tree-iterator.h $(TREE_INLINE_H) $(CGRAPH_H) \
+   $(DIAGNOSTIC_CORE_H) $(GIMPLE_H) $(CILK_H) $(C_COMMON_H) langhooks.h


TREE_H is duplicated.

Also, you are using DIAGNOSTIC_CORE_H whereas c-family/cilk.c is using


+#include diagnostic.h



+/* Cilk Keywords builtins.  */
+#include cilk-builtins.def


keywords should be lowercase


diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index dc430c3..ab77fa4 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -148,6 +148,9 @@ enum rid
   /* C++11 */
   RID_CONSTEXPR, RID_DECLTYPE, RID_NOEXCEPT, RID_NULLPTR, RID_STATIC_ASSERT,

+  /* Cilk Plus Keywords.  */
+  RID_CILK_SPAWN, RID_CILK_SYNC,
+


Same here.


+/* In cilk.c.  */
+extern tree insert_cilk_frame (tree);
+extern void cilk_init_builtins (void);
+extern int gimplify_cilk_spawn (tree *, gimple_seq *, gimple_seq *);
+extern int gimplify_cilk_sync (tree *, gimple_seq *, gimple_seq *);
+extern void c_cilk_install_body_w_frame_cleanup (tree, tree);
+extern bool cilk_valid_spawn (tree *);
+extern bool cilkplus_set_spawn_marker (location_t, tree);


You should be consistent with the prefix throughout the entire patch. 
For instance, now you have cilk_init_builtins() but 
cilkplus_set_spawn_marker().  Do whatever you like, but be consistent.


Also, insert_cilk_frame-- the prefix should be well...as a prefix should 
be...at the beginning.  However, gimplify_cilk_spawn/sync is ok since 
that is in keeping with the rest of the gimplify.c style.



+  /* IF the function has _Cilk_spawn in front of a function call inside it
+ i.e. it is a spawning function, then add the appropriate Cilk plus
+ functions inside.  */


Lowercase the F in IF.


+ c_parser_skip_until_found (parser, CPP_SEMICOLON, expected %;%);
+ if (!flag_enable_cilkplus)
+   error_at (loc, -fcilkplus must be enabled to use _Cilk_sync);


The error message should use %_Cilk_sync


+   case RID_CILK_SPAWN:
+ c_parser_consume_token (parser);
+ if (!flag_enable_cilkplus)
+   {
+ error_at (loc, -fcilkplus must be enabled to use _Cilk_spawn);
+ expr = c_parser_postfix_expression (parser);
+ expr.value = error_mark_node; 


Similarly with _Cilk_spawn.


+ if (c_parser_peek_token (parser)-keyword == RID_CILK_SPAWN)
+   {
+ error_at (input_location, consecutive _Cilk_spawn keywords 
+   are not permitted);


Similarly here-- for that matter, check the rest of the patch for any 
keywords in error messages, they should have %blah.


Also, avoid input_location when possible.  Surely, you can infer the 
actual location from the parser.



+  if (flag_enable_cilkplus  retval  TREE_CODE (retval) == CILK_SPAWN_STMT)
+{
+  error_at (loc, use of _Cilk_spawn in return statement is not allowed);
+  return error_mark_node;


s/in return/in a return/.  Also, use %_Cilk_spawn.


+/* Returns a tree of type CILK_SYNC_STMT if Cilk Plus is enabled.  Otherwise
+   return error_mark_node.  */
+
+tree
+c_build_cilk_sync (void)
+{
+  tree sync = build0 (CILK_SYNC_STMT, void_type_node);
+  TREE_SIDE_EFFECTS (sync) = 1;
+  return sync;
+}


Code seems correct and comment wrong.  Adjust accordingly.


diff --git a/gcc/cilk.h b/gcc/cilk.h
new file mode 100644
index 000..038316a
--- /dev/null
+++ b/gcc/cilk.h
@@ -0,0 +1,94 @@
+/* This file is part of the Intel(R) Cilk(TM) Plus support
+   This file contains Cilk Support files.
+   Copyright (C) 2013  Free Software Foundation, Inc.


Only one space after 2013.  Similarly in other files.


+
+#ifndef GCC_CILK_H
+#define GCC_CILK_H
+
+#include tree.h


As mentioned earlier, cilk.h should not include tree.h, the caller should.


+/* Frame status bits known to compiler.  */
+#define CILK_FRAME_STOLEN0x01


Unused?


+this keyword with a set of functions that are stored in the Cilk Runtime.


Lowercase Runtime.


+@file{c-family/cilk.c}.  In the spawn-helper, the gimplification function
+@code{gimplify_call_expr}, inserts a function call @code{__cilkrts_detach}.
+This function is expanded by @code{builtin_expand_cilk_detach} located in
+@file{c-family/cilk.c}.
+
+@item @code{_Cilk_sync}:
+@code{_Cilk_sync} is parsed 

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Iyer, Balaji V]

Hi Joseph,
The fixed patch and the changelogs are attached in this 
email(http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00758.html). I have 
addressed your concerns below:

Thanks,

Balaji V. Iyer.

 -Original Message-
 From: Joseph Myers [mailto:jos...@codesourcery.com]
 Sent: Friday, August 09, 2013 12:52 PM
 To: Iyer, Balaji V
 Cc: Aldy Hernandez; r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
 Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
 
 On Thu, 8 Aug 2013, Iyer, Balaji V wrote:
 
  +enum add_variable_type  {
 
 Two spaces before '{', should be one.
 
  +static HOST_WIDE_INT cilk_wrapper_count;
 
 This is HOST_WIDE_INT but you use it later with sprintf with %ld; you need to
 use HOST_WIDE_INT_PRINT_DEC in such a case
 
  +  tree map = (tree)*map0;
 
 There are several places like this where you are missing a space in a cast, 
 which
 should be (tree) *map0.

Fixed!

 
  +  /* Build the Cilk Stack Frame:
  + struct __cilkrts_stack_frame {
  +   uint32_t flags;
  +   uint32_t size;
  +   struct __cilkrts_stack_frame *call_parent;
  +   __cilkrts_worker *worker;
  +   void *except_data;
  +   void *ctx[4];
  +   uint32_t mxcsr;
  +   uint16_t fpcsr;
  +   uint16_t reserved;
  +   __cilkrts_pedigree pedigree;
  + };  */
  +
  +  tree frame = lang_hooks.types.make_type (RECORD_TYPE);  tree
  + frame_ptr = build_pointer_type (frame);  tree worker_type =
  + lang_hooks.types.make_type (RECORD_TYPE);  tree worker_ptr =
  + build_pointer_type (worker_type);  tree s_type_node = build_int_cst
  + (size_type_node, 4);
  +
  +  tree flags = add_field (flags, unsigned_type_node, NULL_TREE);
  + tree size = add_field (size, unsigned_type_node, flags);
 
 You refer to some fields as uint32_t above but then build them as unsigned 
 int;
 you should be consistent.
 
Fixed.

 I'm also suspicious of the mxcsr and fpcsr fields and associated enum 
 values.
 They don't really appear to be *used* for anything semantic in this patch -
 there's just boilerplate code dealing with creating them.  So I don't know 
 what
 the point of them is at all - is there an associated runtime using them to be
 added by another patch in this series?  The problem is that they sound
 architecture-specific - they sound like they relate to registers on one 
 particular
 architecture - meaning that they should actually be created by target hooks
 which might create different sets or sizes of such fields on different
 architectures (and they shouldn't appear at all in an enum in architecture-
 independent code, in that case).
 
 
They are removed.

  +  tree mxcsr = add_field (mxcsr, uint32_type_node, context);  tree
  + fpcsr = add_field (fpcsr, uint16_type_node, mxcsr);  tree reserved
  + = add_field (reserved, uint16_type_node, fpcsr);
 
 Note that uint32_type_node and uint16_type_node are internal types that may
 or may not correspond to the stdint.h C typedefs (one could be unsigned int 
 and
 the other unsigned long, for example).  If this matters then you may need to
 work out how to use c_uint32_type_node etc. instead (but this code is outside
 the front end, so that may not be easy to do).
 (Cf. what I said in
 http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00248.html about the
 remaining, presumably unmaintained, targets without stdint.h type information
 in GCC.)
 

  +   int32_t self;
 
  +  field = add_field (self, unsigned_type_node, field);
 
 Again, inconsistency of type.
 

Fixed

  +Used to represent a spawning function in the Cilk Plus language extension.
  +This tree has one field that holds the name of the spawning function.
  +_Cilk_spawn can be written in C in the following way:
 
 @code{_Cilk_spawn} (and likewise _Cilk_sync), in several places.

Fixed.

 
  +Detailed description for usage and functionality of _Cilk_spawn can
  +be found at http://www.cilkplus.org
 
 Use an actual link.
 
We have recently launched the website, so the actual link might change. I will 
change it to the following:
Documentation about Cilk Plus and language specification is provided under the
Learn section in @w{@uref{http://www.cilkplus.org/}}.  

  diff --git a/gcc/tree.h b/gcc/tree.h
  index 0058a4b..952362f 100644
  --- a/gcc/tree.h
  +++ b/gcc/tree.h
  @@ -262,6 +262,7 @@ enum built_in_class
 NOT_BUILT_IN = 0,
 BUILT_IN_FRONTEND,
 BUILT_IN_MD,
  +  BUILT_IN_CILK,
 BUILT_IN_NORMAL
   };
 
  @@ -439,6 +440,8 @@ struct GTY(()) tree_base {
 unsigned protected_flag : 1;
 unsigned deprecated_flag : 1;
 unsigned default_def_flag : 1;
  +  unsigned is_cilk_spawn : 1;
  +  unsigned is_cilk_spawn_detach_point : 1;
 
 No, absolutely not.  This would expand all trees by a whole word.  Find a way 
 to
 do whatever you need without increasing memory consumption like that.
 

OK. Removed.

  @@ -3496,7 +3508,7 @@ struct GTY(()) tree_function_decl {
???  The bitfield needs to be able to hold all target function
codes

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

--- gcc/expr.c
+++ gcc/expr.c
@@ -9569,6 +9569,21 @@ expand_expr_real_1 (tree exp, rtx target, enum

machine_mode tmode,

}

return expand_constructor (exp, target, modifier, false);
+case INDIRECT_REF:
+  {
+   tree exp1 = TREE_OPERAND (exp, 0);
+   if (modifier != EXPAND_WRITE)
+ {
+   tree t = fold_read_from_constant_string (exp);
+   if (t)
+ return expand_expr (t, target, tmode, modifier);
+ }
+   op0 = expand_expr (exp1, NULL_RTX, VOIDmode, EXPAND_SUM);
+   op0 = memory_address (mode, op0);
+   temp = gen_rtx_MEM (mode, op0);
+   set_mem_attributes (temp, exp, 0);
+   return temp;
+  }


Ughhh, what's the rationale for this?  Are generic changes to
expand_expr really needed?


Yes, I am expanding some variables of type ptr-value and those are emitted 
as INDIRECT_REF.


The fact that you are getting an INDIRECT_REF this late in the game is 
suspect.


Are you building with ENABLE_CHECKING, because it seems this should have 
been caught.  See the places in tree-cfg.c with this:


case INDIRECT_REF:
  error (INDIRECT_REF in gimple IL);
  return t;





+  /* During LTO, the is_cilk_function flag gets cleared.
+ If __cilkrts_pop_frame is called, then this definitely must be a
+ cilk function.  */
+  if (cfun)
+cfun-is_cilk_function = 1;


I don't know much about our LTO implementation, but perhaps you need to
teach LTO to stream this bit in/out?  And of course, an accompanying LTO
test to check for this problem you're encountering would be appropriate.



I also have a limited knowledge of LTO. This seem to be the most 
straightforward way of doing it (atleast for me).


See how other bits in `struct function' are streamed in/out in LTO, for 
example in output_struct_function_base()


  bp_pack_value (bp, fn-calls_alloca, 1);
  bp_pack_value (bp, fn-calls_setjmp, 1);
  bp_pack_value (bp, fn-va_list_fpr_size, 8);
  bp_pack_value (bp, fn-va_list_gpr_size, 8);

and the corresponding in input_struct_function_base():

  fn-calls_alloca = bp_unpack_value (bp, 1);
  fn-calls_setjmp = bp_unpack_value (bp, 1);
  fn-va_list_fpr_size = bp_unpack_value (bp, 8);
  fn-va_list_gpr_size = bp_unpack_value (bp, 8);

Also, you will need a testcase to make sure later changes to the 
compiler do not break LTO wrt Cilk features you have added.





+   /* We need frame pointer for all Cilk Plus functions that uses
+ Cilk Keywords.  */
+   || (flag_enable_cilkplus  cfun-is_cilk_function)


need a frame pointer

that use

s/Keywords/keywords/



It should be keywords, because you need frame-pointer for _Cilk_spawn and _Cilk_sync 
and _Cilk_for


I meant that you should lowercase the K.




+  /* This variable will tell whether we are on a spawn helper or not */
+  unsigned int is_cilk_helper_function : 1;


Where is this used?



Well, it is not used now but later on when I add Tools support it will be. I 
will remove it for now.


Yes, please.


--- gcc/ipa-inline-analysis.c
+++ gcc/ipa-inline-analysis.c
@@ -1433,6 +1433,9 @@ initialize_inline_failed (struct cgraph_edge *e)
  e-inline_failed = CIF_REDEFINED_EXTERN_INLINE;
else if (e-call_stmt_cannot_inline_p)
  e-inline_failed = CIF_MISMATCHED_ARGUMENTS;
+  else if (flag_enable_cilkplus  cfun  cfun-calls_spawn)
+/* We can't inline if the function is spawing a function.  */
+e-inline_failed = CIF_BODY_NOT_AVAILABLE;


Hmmm, if we don't have a cfun, perhaps we should be sticking this
calls_spawn bit in the cgraph node.

Richard?  Anyone?



When I am first setting this, I don't think cgraph is available.


See Richard's comment with regards to struct function and its 
availability via the callee edge.  Also see his comment regarding the 
inappropriate error message.



@@ -3496,7 +3510,7 @@ struct GTY(()) tree_function_decl {
   ???  The bitfield needs to be able to hold all target function
  codes as well.  */
ENUM_BITFIELD(built_in_function) function_code : 11;
-  ENUM_BITFIELD(built_in_class) built_in_class : 2;
+  ENUM_BITFIELD(built_in_class) built_in_class  ;


What's this for?



Added a new enum field called BUILT_IN_CILK so we need 3 bits instead of 2, 
since there are 5 fields instead of 4.


Hmm, yeah.  I see you added another field here:


diff --git a/gcc/tree.h b/gcc/tree.h
index 0058a4b..952362f 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -262,6 +262,7 @@ enum built_in_class
   NOT_BUILT_IN = 0,
   BUILT_IN_FRONTEND,
   BUILT_IN_MD,
+  BUILT_IN_CILK,
   BUILT_IN_NORMAL
 };


If you look at the comment above enum built_in_class, you will see that 
these classes specify which part of the compiler created the built-in 
(the frontend, the backend (MD), or a normal builtin).  I don't see how 
Cilk should be treated specially.  And even so, I don't see how you use 
this BUILT_IN_CILK class anywhere.



+  if (DECL_BUILT_IN_CLASS (exp) == BUILT_IN_NORMAL)
+return DECL_FUNCTION_CODE (exp) 

RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Joseph S. Myers]

On Thu, 8 Aug 2013, Iyer, Balaji V wrote:

 +enum add_variable_type  {

Two spaces before '{', should be one.

 +static HOST_WIDE_INT cilk_wrapper_count;

This is HOST_WIDE_INT but you use it later with sprintf with %ld; you need 
to use HOST_WIDE_INT_PRINT_DEC in such a case

 +  tree map = (tree)*map0;

There are several places like this where you are missing a space in a 
cast, which should be (tree) *map0.

 +  /* Build the Cilk Stack Frame:
 + struct __cilkrts_stack_frame {
 +   uint32_t flags;
 +   uint32_t size;
 +   struct __cilkrts_stack_frame *call_parent;
 +   __cilkrts_worker *worker;
 +   void *except_data;
 +   void *ctx[4];
 +   uint32_t mxcsr;
 +   uint16_t fpcsr;
 +   uint16_t reserved;
 +   __cilkrts_pedigree pedigree;
 + };  */
 +
 +  tree frame = lang_hooks.types.make_type (RECORD_TYPE);
 +  tree frame_ptr = build_pointer_type (frame);
 +  tree worker_type = lang_hooks.types.make_type (RECORD_TYPE);
 +  tree worker_ptr = build_pointer_type (worker_type);
 +  tree s_type_node = build_int_cst (size_type_node, 4);
 +
 +  tree flags = add_field (flags, unsigned_type_node, NULL_TREE);
 +  tree size = add_field (size, unsigned_type_node, flags);

You refer to some fields as uint32_t above but then build them as unsigned 
int; you should be consistent.

I'm also suspicious of the mxcsr and fpcsr fields and associated enum 
values.  They don't really appear to be *used* for anything semantic in 
this patch - there's just boilerplate code dealing with creating them.  So 
I don't know what the point of them is at all - is there an associated 
runtime using them to be added by another patch in this series?  The 
problem is that they sound architecture-specific - they sound like they 
relate to registers on one particular architecture - meaning that they 
should actually be created by target hooks which might create different 
sets or sizes of such fields on different architectures (and they 
shouldn't appear at all in an enum in architecture-independent code, in 
that case).

 +  tree mxcsr = add_field (mxcsr, uint32_type_node, context);
 +  tree fpcsr = add_field (fpcsr, uint16_type_node, mxcsr);
 +  tree reserved = add_field (reserved, uint16_type_node, fpcsr);

Note that uint32_type_node and uint16_type_node are internal types that 
may or may not correspond to the stdint.h C typedefs (one could be 
unsigned int and the other unsigned long, for example).  If this matters 
then you may need to work out how to use c_uint32_type_node etc. instead 
(but this code is outside the front end, so that may not be easy to do).  
(Cf. what I said in 
http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00248.html about the 
remaining, presumably unmaintained, targets without stdint.h type 
information in GCC.)

 +   int32_t self;

 +  field = add_field (self, unsigned_type_node, field);

Again, inconsistency of type.

 +Used to represent a spawning function in the Cilk Plus language extension.  
 +This tree has one field that holds the name of the spawning function.
 +_Cilk_spawn can be written in C in the following way:

@code{_Cilk_spawn} (and likewise _Cilk_sync), in several places.

 +Detailed description for usage and functionality of _Cilk_spawn can be found 
 at
 +http://www.cilkplus.org

Use an actual link.

 diff --git a/gcc/tree.h b/gcc/tree.h
 index 0058a4b..952362f 100644
 --- a/gcc/tree.h
 +++ b/gcc/tree.h
 @@ -262,6 +262,7 @@ enum built_in_class
NOT_BUILT_IN = 0,
BUILT_IN_FRONTEND,
BUILT_IN_MD,
 +  BUILT_IN_CILK,
BUILT_IN_NORMAL
  };
  
 @@ -439,6 +440,8 @@ struct GTY(()) tree_base {
unsigned protected_flag : 1;
unsigned deprecated_flag : 1;
unsigned default_def_flag : 1;
 +  unsigned is_cilk_spawn : 1;
 +  unsigned is_cilk_spawn_detach_point : 1;

No, absolutely not.  This would expand all trees by a whole word.  Find a 
way to do whatever you need without increasing memory consumption like 
that.

 @@ -3496,7 +3508,7 @@ struct GTY(()) tree_function_decl {
   ???  The bitfield needs to be able to hold all target function
 codes as well.  */
ENUM_BITFIELD(built_in_function) function_code : 11;
 -  ENUM_BITFIELD(built_in_class) built_in_class : 2;
 +  ENUM_BITFIELD(built_in_class) built_in_class : 3;
  
unsigned static_ctor_flag : 1;
unsigned static_dtor_flag : 1;

Again, no.  See the comment No bits left. at the bottom of this 
structure.  Expanding widely used datastructures is a bad idea, although 
this one isn't as bad to expand as tree_base.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Aldy Hernandez]

[Richard, small question for you below].

Not all your changes to Makefile.in have a changelog entry.


+c-family/cilk.o : c-family/cilk.c $(TREE_H) $(SYSTEM_H) $(CONFIG_H)

toplev.h \

+$(TREE_H) coretypes.h tree-iterator.h $(TREE_INLINE_H)

$(CGRAPH_H) \

+   $(DIAGNOSTIC_CORE_H) $(GIMPLE_H) $(CILK_H)


cilk.c includes langhooks.h and c-family/c-common.h, but I don't see 
dependences for them.



+  if (flag_enable_cilkplus)


We really should do a mass renaming of this flag.  It should be 
flag_cilkplus.  Not necessary to get this patch in, but highly 
desirable, or perhaps as a separate patch.



+  /* IF the function has _Cilk_spawn in front of a function call

inside it

Lowercase IF.  Also, this sentence reads funny.  I don't quite 
understand it.  function call inside it?  Can you reword it?




+/* Creates the internal functions for spawn helper and parent.  */
+
+void
+c_install_body_with_frame_cleanup (tree fndecl, tree body)
+{


Since this is in the global namespace, it needs a Cilk specific name. 
Perhaps cilk_install_body_with_frame_cleanup, or something to that effect?



+  tree list;
+  tree frame = make_cilk_frame (fndecl);
+  tree dtor = build_cilk_function_exit (frame, false, false);
+  add_local_decl (cfun, frame);
+
+  DECL_SAVED_TREE (fndecl) = (list = alloc_stmt_list ());


Split this into two statements.


+ error_at (input_location, consecutive _Cilk_spawn keywords not 
+   permitted);


are not permitted


+
+/* Marks CALL, a CALL_EXPR, as a spawned function call.  */
+
+tree
+c_build_spawns (location_t loc, tree call)
+{
+  cilkplus_set_spawn_marker (loc, call);
+  tree spawn_stmt = build1 (CILK_SPAWN_STMT, TREE_TYPE (call), call);
+  TREE_SIDE_EFFECTS (spawn_stmt) = 1;
+  return spawn_stmt;
+}


First, why the plural?  Second, name should probably be c_build_cilk_spawn


+
+/* Returns a tree of type CILK_SYNC_STMT if Cilk Plus is enabled. Otherwise
+   return error_mark_node.  */


Two spaces after sentence.


+
+tree
+c_build_sync (location_t loc)
+{
+  if (!flag_enable_cilkplus)
+{
+  error_at (loc, -fcilkplus not enabled);
+  return error_mark_node;
+}


If actually needed, the check for cilkplus should be done in the caller.

Also, rename to c_build_cilk_sync


+  if (flag_enable_cilkplus)
+cpp_define_formatted (pfile, __cilk=%d, 200);


Why the %d?  Can't you just hard code the 200 into the string?


+@item CILK_SPAWN_STMT
+
+Used to represent a spawning function in the Cilk Plus language extension. This
+tree has one field that holds the name of the spawning function.
+_Cilk_spawn can be written in C in the following way:


First, two spaces after extension..

Second, you should provide an accessor macro for the operand.  For 
example for a TRANSACTION_EXPR, we have the following in tree.h:


/* TM directives and accessors.  */
#define TRANSACTION_EXPR_BODY(NODE) \
  TREE_OPERAND (TRANSACTION_EXPR_CHECK (NODE), 0)

After you do this, all uses of TREE_OPERAND (blah, 0) should use this 
new accessor, and the documentation here should match.



+ _Cilk_spawn keyword is parsed and the function that it contains is marked as


The _Cilk_spawn keyword...

s/that it/it


+a spawning function.  The spawning function is called the spawner.  At the end
+of the parsing phase, appropriate internal (builtin) functions are added to


Rename internal (builtin) functions to just built-in functions, and 
by the way, the correct nomenclature in GCC is built-in.



+the spawner that are defined in Cilk runtime.  The appropriate locations of


s/in Cilk/in the Cilk/


+these functions, and the internal structures are detailed in
+@code{cilk_init_builtins} in file @file{cilk-common.c}.  The pointers to Cilk


s/in file/in the file/



+functions and fields of internal structures are described in @file{cilk.h}.
+The builtin functions are described in @file{cilk-builtins.def}.


built-in


+
+During the gimplification stage, a new spawn-helper function is created.


You can probably just say during gimplification, ...


+The spawned function is replaced with a spawn helper function in the spawner.
+The spawned function-call is moved into the spawn helper.  The main function
+that does these transformations is @code{gimplify_cilk_spawn} in
+@file{c-family/cilk.c}.  In the spawn-helper, the gimplification function
+@code{gimplify_call_expr}, inserts a function call @code{__cilkrts_detach}.
+This function is expanded by @code{builtin_expand_cilk_detach} located in
+@file{c-family/cilk.c}.
+
+@item _Cilk_sync:
+_Cilk_sync is parsed like any regular keyword.  During gimplification stage,


s/any regular/a
s/During gimplification stage/During gimplification/


+the function @code{gimplify_cilk_sync} in @file{c-family/cilk.c}, will replace
+this keyword with a set of functions that are stored in Cilk Runtime.  One of


in the Cilk run-time


+the internal functions inserted during gimplification stage,


s/gimplification 

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

[Richard Henderson]

On 08/06/2013 06:49 AM, Aldy Hernandez wrote:
 --- gcc/ipa-inline-analysis.c
 +++ gcc/ipa-inline-analysis.c
 @@ -1433,6 +1433,9 @@ initialize_inline_failed (struct cgraph_edge *e)
  e-inline_failed = CIF_REDEFINED_EXTERN_INLINE;
else if (e-call_stmt_cannot_inline_p)
  e-inline_failed = CIF_MISMATCHED_ARGUMENTS;
 +  else if (flag_enable_cilkplus  cfun  cfun-calls_spawn)
 +/* We can't inline if the function is spawing a function.  */
 +e-inline_failed = CIF_BODY_NOT_AVAILABLE;
 
 Hmmm, if we don't have a cfun, perhaps we should be sticking this calls_spawn
 bit in the cgraph node.
 
 Richard?  Anyone?

There will always be a function struct.  Probably not cfun though.
You can get to the callee through the edge.

BODY_NOT_AVAILABLE?  Definitely an odd error message to have chosen
for this...


r~