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

["Iyer, Balaji V" <balaji.v.iyer@intel.com>]

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Wednesday, February 12, 2014 9:59 AM
> To: Iyer, Balaji V
> Cc: 'Jason Merrill'; 'Jeff Law'; 'Aldy Hernandez'; 'gcc-patches@gcc.gnu.org';
> 'rth@redhat.com'
> Subject: Re: [PING] [PATCH] _Cilk_for for C and C++
> 
> On Mon, Feb 10, 2014 at 10:07:18PM +0000, Iyer, Balaji V wrote:
> > I looked at both but forgot to test them with my implementation. Sorry
> > about this.  I have fixed the ICE issue.  To make sure this does not
> > happen further, I have added your test cf3.C into test suite (renamed
> > to cf3.cc).  I hope that is OK with you.
> 
> The testcase is GPL as the original libgomp.c++/for-1.C testcase, so sure.
> Perhaps it would be much better though if instead of having a compile time
> testcase you'd just do what libgomp.c++/for-1.C does, just replace all the
> #pragma omp parallel for in there with _Cilk_for and turn it into a runtime
> testcase.
> 
I really don't want to do that because I don't think there is a 1:1 match-up between the rules of #pragma omp for and _Cilk_for. 

> > I have attached a fixed patch and Changelogs. Is this OK?
> 
> Looks better (note, still looking just at the dumps), but not completely ok
> yet.  On cf3.cc, I see in *.gimple:
> 
>         D.2883 = J<int>::begin (j);
>         I<int>::I (&i, D.2883);
>         D.2885 = J<int>::end (j);
>         retval.0 = operator-<int> (D.2885, &i);
>         D.2886 = retval.0 / 2;
>         #pragma omp parallel firstprivate(i) if(D.2886) shared(D.2865) shared(j)
>           {
>             difference_type retval.1;
>             const struct I & D.2888;
>             const difference_type D.2866;
>             long int D.2889;
>             struct I & D.2890;
> 
>             try
>               {
> 
>                 _Cilk_for (D.2864 = 0; D.2864 < retval.1; D.2864 = D.2864 + 2)
> private(D.2864)
>                   {
>                     D.2889 = D.2864 - D.2865;
>                     D.2866 = D.2889;
>                     try
>                       {
>                         D.2890 = I<int>::operator+= (&i, &D.2866);
> 
> First a minor nit, there is extra newline before _Cilk_for:
>           newline_and_indent (buffer, spc);
>           if (flag_cilkplus
>               && gimple_omp_for_kind (gs) == GF_OMP_FOR_KIND_CILKFOR)
>             pp_string (buffer, "_Cilk_for (");
>           else
>             pp_string (buffer, "for ("); I guess for _Cilk_for collapse is never > 1,
> right?  If that is the case, then perhaps you should move the
> newline_and_indent (buffer, spc); call into the else block.
> 

OK. I will fix this and send you a patch? 

> More importantly, what is retval.1?  I'd expect you should be using retval.0
> there and have it also as firstprivate(retval.0) on the parallel.
> In *.omplower dump I actually see:
>         retval.0 = operator-<int> (D.2885, &i); ...
>                             retval.1 = operator-<int> (D.2888, &i); i.e. operator-<int> is
> called twice.
> 

Yes, one is for the if-clause and one is for the condition. It really doesn't matter because we get of the stuff in the condition and replace with our own for loop with something like the for-loop shown  below. So retval1 doesn't come into picture. It is only alive from parser to the expand_cilk_for function.

For (i = low; i < high; i++)
{
	<_Cilk_for_body>
}

So, is there any other changes that you need me to make?

Thanks,

Balaji V. Iyer.


> 	Jakub