On 07/08/15 10:58, Jakub Jelinek wrote:
> On Wed, Jul 08, 2015 at 10:47:56AM -0400, Nathan Sidwell wrote:
>> +/* Generate loop head markers in outer->inner order.  */
>> +
>> +static void
>> +gen_oacc_fork (gimple_seq *seq, unsigned mask)
>> +{
>> +  {
>> +    // TODDO: Determine this information from the parallel region itself
>
> TODO ?

I want to clean this up with the offloading launch API.  As it happens, I did 
manage to have the PTX  backend DTRT if it doesn't encounter this internal fn. 
I'm dropping it fromm this patch (it'd undoubtedly need moving around anyway).

>> +	gcall *call = gimple_build_call_internal
>> +	  (IFN_GOACC_FORK, 1, arg);
>
> Why the line-break?  That should fit into 80 columns just fine.

oh, it does now I've changed the name of the internal function.

>> + It'd be better to place the OACC_LOOP markers just inside the outer
>> + conditional, so they can be entirely eliminated if the loop is
>> + unreachable.
>
> Putting OACC_FORK/OACC_JOIN unconditionally into the comment is very
> confusing.  The expand_omp_for_static_nochunk routine is used for
> #pragma omp for schedule(static), #pragma omp distribute etc. which
> certainly don't want to emit such markers in there.  So perhaps mention
> somewhere that you wrap all the above sequence in between
> OACC_FORK/OACC_JOIN markers.

Done. (at both sites)

> Please avoid such whitespace changes.

Fixed (& searched others).

> In any case, as it is a gomp-4_0-branch patch, I'll defer full review to the
> branch maintainers.

Thanks for your review!

nathan