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

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

Hi Jason,
	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)

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jason Merrill [mailto:jason@redhat.com]
> Sent: Thursday, January 16, 2014 12:30 PM
> To: Iyer, Balaji V; Jakub Jelinek
> Cc: 'Jeff Law'; 'Aldy Hernandez'; 'gcc-patches@gcc.gnu.org'; 'rth@redhat.com'
> Subject: Re: [PATCH] _Cilk_for for C and C++
> 
> On 01/08/2014 02:46 PM, Iyer, Balaji V wrote:
> > +  /* Grain value, only used by _Cilk_for.  */  tree grain;
> 
> Why can't the grain stay as a clause for the gimple form of the loop?
> 

Ok. Removed the field and modified the code accordingly.

> > +  if (flag_enable_cilkplus && TREE_CODE (for_stmt) == CILK_FOR)
> > +    {
> > +      tree it = TREE_VEC_ELT (OMP_FOR_INIT (for_stmt), 0);
> > +      gimplify_and_add (it, &for_pre_body);
> > +    }
> 
> Why doesn't the normal handling of OMP_FOR_INIT work for Cilk?  All the
> special cases for CILK_FOR need comments explaining why they are needed.
> 
Removed. It was an old artifact

> Also, this seems like you're assigning to the control variable outside of the
> loop, which doesn't makes sense because we initialize it in each of the
> invocations of the child function.  Right?
> 

I am just saving the values to a temporary variable to hold the 

> > +      /* Original initial, final and increment values are necessary to compute
> > +	 the loop-count.  Otherwise, they are stored in variables and their
> > +	 context could be changed, potentially making it impossible to
> compute
> > +	 them correctly.  */
> 
> I don't understand.  Surely all you care about is the value, and gimplification
> shouldn't affect that.
>
> > +	  /* If VAR is the induction variable of the outer _Cilk_for, then
> > +	     it needs to be passed as a value not pointer since it
> > +	     would not be overwritten by the body.  */
> 
> Here it looks like you're overriding the normal logic because we know that it's
> safe to assume the induction variable won't be changed by the body of the
> loop.  But why is the induction variable shared in the first place?  If it isn't
> going to change, it can be private.
> 

In OMP, when we have nested fors induction variable is passed in as an address. For us we just need it passed as value since we modify it. The original induction variable is replaced (in value) by the new induction variable inserted in the child function body. This is done here to fix that.

> > +	/* We cannot use the tsubst_omp_clauses since it will try to
> > +	   do checking such as whether a certain clause can be used
> > +	   with a certain for-loop.  We are just use schedule clause here
> > +	   as a holder to hold the grain value.  */
> 
> I don't see the checking you mention.  Can't we fix it to do the right thing?
> 

This again was an artifact. This is removed also.

> > +  if (code == CILK_FOR)
> > +    {
> > +      top_level_body = push_stmt_list ();
> > +      top_body = begin_omp_parallel ();
> > +    }
> 
> I wouldn't expect the front end to care that Cilk for is implemented using a
> parallel call; can't we bring that in at lowering time?
> 

The way I have implemented _Cilk_for is like a for-loop where the for-statement excluding the body is like a #pragma omp for and the body itself is #pragma omp parallel. This is sort of backwards from the OMP's for loops. This way pulling body into a child function, etc. is done by the existing infrastructure. The child function is modified accordingly to fit _Cilk_for in a function called expand_cilk_for_body that is called in expand_omp_taskreg . During expand_omp_for we just insert the call to the runtime function.

I am sure there are other ways to do this, but this seem to be the most straightforward way to do this with the least amount of code-change/duplication.

> Jason

Attachment: c-ChangeLog
Description: c-ChangeLog

Attachment: cp-ChangeLog
Description: cp-ChangeLog

Attachment: diff_c.txt
Description: diff_c.txt

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