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
