This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH,nvptx] Use CUDA driver API to select default runtime launch, geometry


On 08/13/2018 05:04 AM, Tom de Vries wrote:
> On 08/10/2018 08:39 PM, Cesar Philippidis wrote:
>> is that I modified the default value for vectors as follows
>>
>> +	    int vectors = default_dim_p[GOMP_DIM_VECTOR]
>> +	      ? 0 : dims[GOMP_DIM_VECTOR];
>>
>> Technically, trunk only supports warp-sized vectors, but the fallback
>> code is already checking for the presence of vectors as so
>>
>> +	    if (default_dim_p[GOMP_DIM_VECTOR])
>> +	      dims[GOMP_DIM_VECTOR]
>> +		= MIN (dims[GOMP_DIM_VECTOR],
>> +		       (targ_fn->max_threads_per_block / warp_size
>> +			* warp_size));
>>
> 
> That code handles the case that the default vector size is bigger than
> the function being launched allows, independent from whether that
> default is calculated by the runtime, or set by GOMP_OPENACC_DIM.
> 
> The GOMP_OPENACC_DIM part is forward compatible, given that currently
> the compiler doesn't allow the runtime to choose the vector length, and
> AFAICT that will remain the same after application of the submitted set
> of vector_length patches.
> 
>> therefore, I had the cuOccupancyMaxPotentialBlockSize code path behave
>> the same.
> 
> They don't behave the same. What you add here is ignoring
> GOMP_OPENACC_DIM[GOMP_DIM_VECTOR], not handling it. That requires a comment.

I meant, same in the sense that it inspects for a pre-defined value of
vector length; not the application of vector length. I should have been
more clear.

> Furthermore, by assigning dims[GOMP_DIM_VECTOR] at the start you break
> the pattern of the code, which:
> - first applies GOMP_OPENACC_DIM
> - then further fills in defaults as required
> - then applies defaults
> I've rewritten this bit to fit the pattern. This result is not pretty,
> but it'll do for now.  Changing the pattern may make things better
> structured, but this is something we can do in a follow up patch, and
> want to do for all dimensions at once, not just for vector, otherwise
> the code will become too convoluted.
> 
> Btw, I've also noticed that we don't handle a too high
> GOMP_OPENACC_DIM[GOMP_DIM_WORKER], I've added a TODO comment for this.

That's why I set vectors to dims[GOMP_DIM_VECTOR] when set. However, I
do agree that this is a task for a follow up patch.

> Committed as attached.

Thank you Tom!

Looking at my patch queue, there's only one more non-vector length
related patch in there - Remove use of CUDA unified memory in libgomp
<https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01970.html>. Going
forward, how would you like to proceed with the nvptx BE vector length
changes.

Cesar


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]