On Tue, 8 Dec 2020, Julian Brown wrote:
> Ping? This has addressed my concerns, thanks. Alexander > On Fri, 13 Nov 2020 20:54:54 +0000 > Julian Brown <jul...@codesourcery.com> wrote: > > > Hi Alexander, > > > > Thanks for the review! Comments below. > > > > On Tue, 10 Nov 2020 00:32:36 +0300 > > Alexander Monakov <amona...@ispras.ru> wrote: > > > > > On Mon, 26 Oct 2020, Jakub Jelinek wrote: > > > > > > > On Mon, Oct 26, 2020 at 07:14:48AM -0700, Julian Brown wrote: > > > > > This patch adds caching for the stack block allocated for > > > > > offloaded OpenMP kernel launches on NVPTX. This is a performance > > > > > optimisation -- we observed an average 11% or so performance > > > > > improvement with this patch across a set of accelerated GPU > > > > > benchmarks on one machine (results vary according to individual > > > > > benchmark and with hardware used). > > > > > > In this patch you're folding two changes together: reuse of > > > allocated stacks and removing one host-device synchronization. Why > > > is that? Can you report performance change separately for each > > > change (and split out the patches)? > > > > An accident of the development process of the patch, really -- the > > idea for removing the post-kernel-launch synchronisation came from the > > OpenACC side, and adapting it to OpenMP meant the stacks had to remain > > allocated after the return of the GOMP_OFFLOAD_run function. > > > > > > > A given kernel launch will reuse the stack block from the > > > > > previous launch if it is large enough, else it is freed and > > > > > reallocated. A slight caveat is that memory will not be freed > > > > > until the device is closed, so e.g. if code is using highly > > > > > variable launch geometries and large amounts of GPU RAM, you > > > > > might run out of resources slightly quicker with this patch. > > > > > > > > > > Another way this patch gains performance is by omitting the > > > > > synchronisation at the end of an OpenMP offload kernel launch -- > > > > > it's safe for the GPU and CPU to continue executing in parallel > > > > > at that point, because e.g. copies-back from the device will be > > > > > synchronised properly with kernel completion anyway. > > > > > > I don't think this explanation is sufficient. My understanding is > > > that OpenMP forbids the host to proceed asynchronously after the > > > target construct unless it is a 'target nowait' construct. This may > > > be observable if there's a printf in the target region for example > > > (or if it accesses memory via host pointers). > > > > > > So this really needs to be a separate patch with more explanation > > > why this is okay (if it is okay). > > > > As long as the offload kernel only touches GPU memory and does not > > have any CPU-visible side effects (like the printf you mentioned -- I > > hadn't really considered that, oops!), it's probably OK. > > > > But anyway, the benefit obtained on OpenMP code (the same set of > > benchmarks run before) of omitting the synchronisation at the end of > > GOMP_OFFLOAD_run seems minimal. So it's good enough to just do the > > stacks caching, and miss out the synchronisation removal for now. (It > > might still be something worth considering later, perhaps, as long as > > we can show some given kernel doesn't use printf or access memory via > > host pointers -- I guess the former might be easier than the latter. I > > have observed the equivalent OpenACC patch provide a significant boost > > on some benchmarks, so there's probably something that could be gained > > on the OpenMP side too.) > > > > The benefit with the attached patch -- just stacks caching, no > > synchronisation removal -- is about 12% on the same set of benchmarks > > as before. Results are a little noisy on the machine I'm benchmarking > > on, so this isn't necessarily proof that the synchronisation removal > > is harmful for performance! > > > > > > > In turn, the last part necessitates a change to the way > > > > > "(perhaps abort was called)" errors are detected and reported. > > > > > > > > > > > As already mentioned using callbacks is problematic. Plus, I'm sure > > > the way you lock out other threads is a performance loss when > > > multiple threads have target regions: even though they will not run > > > concurrently on the GPU, you still want to allow host threads to > > > submit GPU jobs while the GPU is occupied. > > > > > > I would suggest to have a small pool (up to 3 entries perhaps) of > > > stacks. Then you can arrange reuse without totally serializing host > > > threads on target regions. > > > > I'm really wary of the additional complexity of adding a stack pool, > > and the memory allocation/freeing code paths in CUDA appear to be so > > slow that we get a benefit with this patch even when the GPU stream > > has to wait for the CPU to unlock the stacks block. Also, for large > > GPU launches, the size of the soft-stacks block isn't really trivial > > (I've seen something like 50MB on the hardware I'm using, with default > > options), and multiplying that by 3 could start to eat into the GPU > > heap memory for "useful data" quite significantly. > > > > Consider the attached (probably not amazingly-written) microbenchmark. > > It spawns 8 threads which each launch lots of OpenMP kernels > > performing some trivial work, then joins the threads and checks the > > results. As a baseline, with the "FEWER_KERNELS" parameters set (256 > > kernel launches over 8 threads), this gives us over 5 runs: > > > > real 3m55.375s > > user 7m14.192s > > sys 0m30.148s > > > > real 3m54.487s > > user 7m6.775s > > sys 0m34.678s > > > > real 3m54.633s > > user 7m20.381s > > sys 0m30.620s > > > > real 3m54.992s > > user 7m12.464s > > sys 0m29.610s > > > > real 3m55.471s > > user 7m14.342s > > sys 0m29.815s > > > > With a version of the attached patch, we instead get: > > > > real 3m53.404s > > user 3m39.869s > > sys 0m16.149s > > > > real 3m54.713s > > user 3m41.018s > > sys 0m16.129s > > > > real 3m55.242s > > user 3m55.148s > > sys 0m17.130s > > > > real 3m55.374s > > user 3m40.411s > > sys 0m15.818s > > > > real 3m55.189s > > user 3m40.144s > > sys 0m15.846s > > > > That is: real time is about the same, but user/sys time are reduced. > > > > Without FEWER_KERNELS (1048576 kernel launches over 8 threads), the > > baseline is: > > > > real 12m29.975s > > user 24m2.244s > > sys 8m8.153s > > > > real 12m15.391s > > user 23m51.018s > > sys 8m0.809s > > > > real 12m5.424s > > user 23m38.585s > > sys 7m47.714s > > > > real 12m10.456s > > user 23m51.691s > > sys 7m54.324s > > > > real 12m37.735s > > user 24m19.671s > > sys 8m15.752s > > > > And with the patch, we get: > > > > real 4m42.600s > > user 16m14.593s > > sys 0m40.444s > > > > real 4m43.579s > > user 15m33.805s > > sys 0m38.537s > > > > real 4m42.211s > > user 16m32.926s > > sys 0m40.271s > > > > real 4m44.256s > > user 15m49.290s > > sys 0m39.116s > > > > real 4m42.013s > > user 15m39.447s > > sys 0m38.517s > > > > Real, user and sys time are all dramatically less. So I'd suggest that > > the attached patch is an improvement over the status quo, even if we > > could experiment with the stacks pool idea as a further improvement > > later on. > > > > The attached patch also implements a size limit for retention of the > > soft-stack block -- freeing it before allocating more memory, rather > > than at the start of a kernel launch, so bigger blocks can still be > > shared between kernel launches if there's no memory allocation between > > them. It also tries freeing smaller cached soft-stack blocks and > > retrying memory allocation in out-of-memory situations. > > > > Re-tested with offloading to NVPTX. OK for trunk? > > > > Thanks, > > > > Julian > > > > ChangeLog > > > > 2020-11-13 Julian Brown <jul...@codesourcery.com> > > > > libgomp/ > > * plugin/plugin-nvptx.c (SOFTSTACK_CACHE_LIMIT): New define. > > (struct ptx_device): Add omp_stacks struct. > > (nvptx_open_device): Initialise cached-stacks housekeeping info. > > (nvptx_close_device): Free cached stacks block and mutex. > > (nvptx_stacks_free): New function. > > (nvptx_alloc): Add SUPPRESS_ERRORS parameter. > > (GOMP_OFFLOAD_alloc): Add strategies for freeing soft-stacks > > block. (nvptx_stacks_alloc): Rename to... > > (nvptx_stacks_acquire): This. Cache stacks block between runs if > > same size or smaller is required. > > (nvptx_stacks_free): Remove. > > (GOMP_OFFLOAD_run): Call nvptx_stacks_acquire and lock stacks > > block during kernel execution. >
