Re: [C++ PATCH] [gomp4] Initial OpenACC support to C++ front-end

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Thu, 13 Mar 2014, Ilmir Usmanov wrote:

> 	* gcc/testsuite/c-c++-common/goacc/deviceptr-1.c: Move to ...
> 	* gcc/testsuite/gcc.dg/goacc/deviceptr-1.c ... here.
> 	* gcc/testsuite/g++.dg/goacc/goacc.exp: New test directory.
> 	* gcc/testsuite/g++.dg/goacc-gomp/goacc-gomp.exp: Likewise.

The ChangeLog file is in gcc/testsuite/, so paths should be given
relative to that directory (i.e. without the gcc/testsuite/ part).

> 	gcc/testsuite/g++.dg/goacc/
> 	* deviceptr-1.cpp: New test.
> 	* sb-1.cpp: Likewise.
> 	* sb-2.cpp: Likewise. 

Here, each entry should contain the g++.dg/goacc/ part.  And the
ChangeLog entry should be updated for the change in filenames to *.C.

> +  for (t = vars; t && t; t = TREE_CHAIN (t))

This use of "t && t" seems odd.

> +	 c_parser_omp_var_list_parens() should construct a list of

No use of () when referring to a function in a comment.

> +static tree
> +cp_parser_oacc_all_clauses (cp_parser *parser, omp_clause_mask mask,
> +			   const char *where, cp_token *pragma_tok, 
> +			   bool finish_p = true)

No caller seems to set this finish_p argument, so I don't see a need
for it.

> +/* OpenACC 2.0:
> +   # pragma acc data oacc-data-clause[optseq] new-line
> +     structured-block
> +
> +   LOC is the location of the #pragma token.
> +*/

> +static tree
> +cp_parser_oacc_data (cp_parser *parser, cp_token *pragma_tok)

There's no parameter LOC, so it seems wrong for the comment to mention
one.  (This applies to other functions with such a comment as well.)

Observations on the tests: I don't see anything testing diagnostics
for the case where it's a return statement that branches out of a
block for which isn't not permitted (you have tests for goto and
switch statements doing such branches) - is that because such tests
are also missing for C?

There are questions of how OpenACC constructs interact with C++
features not present in C.  I think a lot of such questions would
apply more to the implementation of the routine directive than to the
things in this patch (as there may well be C++ features not readily
supported on an accelerator).  For the features in this patch, I
suppose exception handling is another form of invalid jump out of a
structured block, but it must be considered undefined behavior at
runtime because it can't be detected at compile time.  I guess
something to include in the testsuite is testing use of OpenACC
directives within templates.  Thus, you have a diagnostic for
non-pointer variables being used in a deviceptr clause; the testsuite
should verify that if the clause is used within a template, and the
type of the variable depends on the type for which a template is
instantiated, you only get the error for an instantiation giving it a
non-pointer type, not if all instantiations give it a pointer type.
(Generally, this applies to any check of something that can only be
determined for a particular instantiation.)

-- 
Joseph S. Myers
joseph@codesourcery.com