RE: [PATCH] _Cilk_for for C and C++

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

Hi Jakub,
	I am attaching the latest patches with the pretty print fixes that you mentioned for Cilk_for. Nothing else has changed except that I have rebased with the latest trunk revision (r207047). I have tested on x86_64 on 32 and 64 bit mode and it passes all tests and does not disrupt any other tests.

	Please let me know if it is OK to trunk.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Thursday, January 23, 2014 5:13 AM
> To: Iyer, Balaji V
> Cc: Jason Merrill; 'Jeff Law'; 'Aldy Hernandez'; 'gcc-patches@gcc.gnu.org';
> 'rth@redhat.com'
> Subject: Re: [PATCH] _Cilk_for for C and C++
> 
> On Sun, Jan 19, 2014 at 04:50:39AM +0000, Iyer, Balaji V wrote:
> > I have answered your questions below. In addition to your changes, I
> > have also fixed the issues Aldy pointed out and have answered his
> > questions in that thread.  With this email I have attached two patches
> > and 2 change-logs (for C and C++).  I have also rebased these patches
> > to the trunk revision (r206756)
> 
> Haven't looked at the patch yet, just applied and looked what it generates:
> 
> Say in cilk_fors.c -O2 -fcilkplus -std=c99 -fdump-tree-
> {original,gimple,omplower,ompexp}
> I'm seeing in *.original dump:
>     <<< Unknown tree: cilk_for
> which suggests that tree-pretty-print.c doesn't handle CILK_FOR.
> 
> Much more important is what is seen in the *.gimple dump though:
>            schedule(runtime,0) private(ii)
>           _Cilk_for (ii = 0; ii <= 9; ii = ii + 1)
>             {
>               #pragma omp parallel shared(__cilk_incr.0) shared(__cilk_cond.2)
> shared(__cilk_init.1) shared(ii) shared(Array)
>                 {
>                   Array[ii] = 1133;
>                 }
>             }
> Why do you put the parallel inside of _Cilk_for rather than the other way
> around?  That looks just wrong.  That would represent runtime scheduling of
> work across the parallel regions at the _Cilk_for, and then in each iteration
> running the body in several threads concurrently.
> You want the parallel around the _Cilk_for, and
> gimple_omp_parallel_set_combined_p (parallel_stmt, true) so that you can
> then handle it specially during omp lowering/expansion.
> 
> Also, the printing of _Cilk_for is weird, the clauses (with space before) look
> really weird above the _Cilk_for when there is no #pragma or something
> similar.  Perhaps print the clauses after _Cilk_for?
>   _Cilk_for (ii = 0; ii <= 9; ii = ii + 1) schedule(runtime,0) private(ii) ?
> 
> 	Jakub

Attachment: diff_c++.txt
Description: diff_c++.txt

Attachment: diff_c.txt
Description: diff_c.txt

Attachment: cp-ChangeLog
Description: cp-ChangeLog

Attachment: c-ChangeLog
Description: c-ChangeLog