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: [openacc] tile, independent, default, private and firstprivate support in c/++


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


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