On 11/04/2015 11:29 PM, Jakub Jelinek wrote: > On Wed, Nov 04, 2015 at 08:58:32PM -0800, Cesar Philippidis wrote: >> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >> index e3f55a7..4424596 100644 >> --- a/gcc/cp/pt.c >> +++ b/gcc/cp/pt.c >> @@ -14395,6 +14395,15 @@ tsubst_omp_clauses (tree clauses, bool >> declare_simd, bool allow_fields, >> case OMP_CLAUSE_PRIORITY: >> case OMP_CLAUSE_ORDERED: >> case OMP_CLAUSE_HINT: >> + case OMP_CLAUSE_NUM_GANGS: >> + case OMP_CLAUSE_NUM_WORKERS: >> + case OMP_CLAUSE_VECTOR_LENGTH: >> + case OMP_CLAUSE_GANG: > > GANG has two arguments, so you want to handle it differently, you need > to tsubst both arguments.
Good catch. Support for the static argument was added after I added template support in gomp4. I'll fix that. >> + case OMP_CLAUSE_WORKER: >> + case OMP_CLAUSE_VECTOR: >> + case OMP_CLAUSE_ASYNC: >> + case OMP_CLAUSE_WAIT: >> + case OMP_CLAUSE_TILE: > > I don't think tile will work well this way, if the only argument is a > TREE_VEC, then I think you hit: > case TREE_VEC: > /* A vector of template arguments. */ > gcc_assert (!type); > return tsubst_template_args (t, args, complain, in_decl); > which does something very much different from making a copy of the TREE_VEC > and calling tsubst_expr on each argument. > Thus, either you need to handle it manually here, or think about different > representation of OMP_CLAUSE_TILE? It seems you allow at most one tile > clause, so perhaps you could split the single source tile clause into one > tile clause per expression in there (the only issue is that the FEs > emit the clauses in last to first order, so you'd need to nreverse the > tile clause list before adding it to the list of all clauses). It shouldn't be difficult to call it manually here. > Otherwise it looks ok, except: How about the other patch, with the c and c++ FE changes? Is that one OK for trunk now? Nathan's going to need it for this firstprivate changes in the middle end. >> diff --git a/gcc/testsuite/g++.dg/goacc/template-reduction.C >> b/gcc/testsuite/g++.dg/goacc/template-reduction.C >> new file mode 100644 >> index 0000000..668eeb3 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/goacc/template-reduction.C >> +++ b/gcc/testsuite/g++.dg/goacc/template.C > > the testsuite coverage is orders of magnitude smaller than it should be. > Just look at the amount of OpenMP template tests (both compile time and > runtime): > grep template libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep -l template > libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep template > gcc/testsuite/g++.dg/gomp/* | wc -l; grep -l template > gcc/testsuite/g++.dg/gomp/* | wc -l > 629 # templates > 45 # tests with templates > 151 # templates > 58 # tests with templates > and even that is really not sufficient. From my experience, special care is > needed in template tests to test both non-type dependent and type-dependent > cases (e.g. some diagnostics is emitted already when parsing the templates > even when they won't be instantiated at all, other diagnostic is done during > instantiation), or for e.g. references there are interesting cases where > a reference to template parameter typename is used or where a reference to > some time is tsubsted into a template parameter typename. > E.g. you don't have any test coverage for the vector (num: ...) > or gang (static: *, num: type_dep) > or gang (static: type_dep1, type_dep2) > (which would show you the above issue with the gang clause), or sufficient > coverage for tile, etc. > Of course that coverage can be added incrementally. We'll add more tests incrementally. Cesar