Hi! On Fri, 5 Oct 2018 21:03:36 +0100, Julian Brown <jul...@codesourcery.com> wrote: > Continuing the thread from here: > > https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00198.html > > On Wed, 3 Feb 2016 19:52:09 +0300 > Alexander Monakov <amona...@ispras.ru> wrote: > > > On Wed, 3 Feb 2016, Nathan Sidwell wrote: > > > You can only override at runtime those dimensions that you said > > > you'd override at runtime when you compiled your program. > > > > Ah, I see. That's not obvious to me, so perhaps added documentation > > can be expanded to explain that? (I now see that the plugin silently > > drops user-provided dimensions where a value recorded at compile time > > is present; not sure if that'd be worth a runtime diagnostic, could > > be very noisy) > > This version of the patch has slightly-expanded documentation.
Thanks! > While the runtime part of the patch already appears to have been > committed as part of the following patch: > > https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01589.html (Not really as part of that one, but as part of other commits.) > The compile-time part of the patch has not made it upstream yet. Thus, > this rebased and retested patch consists of the parsing changes (for > -fopenacc-dim=X:Y:Z, allowing '-') That seems reasonable for next GCC development stage 1, with changes as per below. But please make sure that adequate testsuite coverage is present for this functionality. > and warning changes (for strange > partitioning choices), plus associated testsuite adjustments. These changes I'd like to defer, that's a separate topic. The general intention is good, but I've seen cases where I considered these diagnostics to be too noisy. See also the several 'dg-bogus' with XFAIL that you're proposing to add? We should think about that some more. So, the following is relevant right now: > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -2167,8 +2167,12 @@ have support for @option{-pthread}. > @cindex OpenACC accelerator programming > Specify default compute dimensions for parallel offload regions that do > not explicitly specify. The @var{geom} value is a triple of > -':'-separated sizes, in order 'gang', 'worker' and, 'vector'. A size > -can be omitted, to use a target-specific default value. > +':'-separated sizes, in order 'gang', 'worker' and, 'vector'. If a size > +is to be deferred until execution '-' can be used, alternatively a size > +can be omitted to use a target-specific default value. When deferring > +to runtime, the environment variable @var{GOMP_OPENACC_DIM} can be set. > +It has the same format as the option value, except that '-' is not > +permitted. ACK. I re-discovered that 'GOMP_OPENACC_DIM' is not currently documented in the libgomp manual (also referring back to the compile-time flag). That can be fixed incrementally, later on; that's PR85129 "[openacc] Document GOMP_OPENACC_DIM". > --- a/gcc/omp-offload.c > +++ b/gcc/omp-offload.c > @@ -574,8 +574,9 @@ static int oacc_default_dims[GOMP_DIM_MAX]; > static int oacc_min_dims[GOMP_DIM_MAX]; > > /* Parse the default dimension parameter. This is a set of > - :-separated optional compute dimensions. Each specified dimension > - is a positive integer. When device type support is added, it is > + :-separated optional compute dimensions. Each dimension is either > + a positive integer, or '-' for a dynamic value computed at > + runtime. When device type support is added, it is > planned to be a comma separated list of such compute dimensions, > with all but the first prefixed by the colon-terminated device > type. */ > @@ -610,14 +611,20 @@ oacc_parse_default_dims (const char *dims) > > if (*pos != ':') > { > - long val; > - const char *eptr; > + long val = 0; Don't set "val = 0" here, but instead... > > - errno = 0; > - val = strtol (pos, CONST_CAST (char **, &eptr), 10); > - if (errno || val <= 0 || (int) val != val) > - goto malformed; > - pos = eptr; > + if (*pos == '-') ... do that here, so that it's clear that this is how the '-' case gets encoded. > + pos++; > + else > + { > + const char *eptr; > + > + errno = 0; > + val = strtol (pos, CONST_CAST (char **, &eptr), 10); > + if (errno || val <= 0 || (int) val != val) > + goto malformed; > + pos = eptr; > + } > oacc_default_dims[ix] = (int) val; > } > } > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/loop-default-compile.c > @@ -0,0 +1,13 @@ > +/* { dg-additional-options "-fopenacc-dim=16:16" } */ > +/* This code uses nvptx inline assembly guarded with acc_on_device, which is > + not optimized away at -O0, and then confuses the target assembler. > + { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */ You no longer need to skip for '-O0'. > +/* { dg-set-target-env-var "GOMP_OPENACC_DIM" "8:8" } */ > + > +#include "loop-default.h" > + > +int main () > +{ > + /* Environment should be ignored. */ > + return test_1 (16, 16, 32); > +} Does that single test case constitute sufficient testsuite coverage? Well, it doesn't even test the '-' case? If that's a useful test on its own, then commit it on its own. (I have not researched its history, and why it's not in trunk yet.) Probably some of the 'dg-additional-options' '-fopenacc-dim' changes with '-' that appear in: $ git diff upstream/trunk..upstream-git/openacc-gcc-8-branch -- libgomp/testsuite/libgomp.oacc-c-c++-common/pr85486\* libgomp/testsuite/libgomp.oacc-c-c++-common/vector-length-128-\* libgomp/testsuite/libgomp.oacc-fortran/gemm.f90 ... should be part of this patch, to add some basic test coverage? (Unless, of course, there's a reason that the '-fopenacc-dim' flags in trunk are different, intentionally, which I have not researched.) Grüße Thomas
signature.asc
Description: PGP signature
