subject:"\[patch,openacc\] use firstprivate pointers for subarrays in c and c\+\+"

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-06-01 Thread Jakub Jelinek

On Wed, Jun 01, 2016 at 10:33:32AM -0700, Cesar Philippidis wrote:
> >>switch (OMP_CLAUSE_MAP_KIND (c))
> >>  {
> >>  case GOMP_MAP_ALLOC:
> > | case GOMP_MAP_TO:
> > | case GOMP_MAP_FROM:
> > | case GOMP_MAP_TOFROM:
> > | case GOMP_MAP_ALWAYS_TO:
> > | case GOMP_MAP_ALWAYS_FROM:
> > | case GOMP_MAP_ALWAYS_TOFROM:
> > | case GOMP_MAP_RELEASE:
> > | case GOMP_MAP_DELETE:
> > |   OMP_CLAUSE_MAP_MAYBE_ZERO_LENGTH_ARRAY_SECTION (c) = 1;
> > |   break;
> > | default:
> >>break;
> >>  }
> > 
> > Why doesn't that apply also to the other (OpenACC) map kinds?  Comparing
> > to the full list in include/gomp-constants.h:enum gomp_map_kind, there
> > are several missing here.
> 
> It does look like there are situations where OpenACC can take
> zero-length arrays, e.g. when the length argument is a variable. This
> will be fixed in my follow up patch.

The question is if you need/want the OpenMP 4.5 mandated handling of zero length
array sections for those, where mapping of zero length array section (unlike
e.g. GNU extension zero length object) doesn't actually map it if not
already mapped, but sets the corresponding pointer to NULL.  If already
mapped, it increments refcount. 

> >> @@ -6054,7 +6070,7 @@ finish_omp_clauses (tree clauses, enum 
> >> c_omp_region_type ort)
> >>omp_note_field_privatization (t, OMP_CLAUSE_DECL (c));
> >>  else
> >>t = OMP_CLAUSE_DECL (c);
> >> -if (t == current_class_ptr)
> >> +if (ort != C_ORT_ACC && t == current_class_ptr)
> >>{
> >>  error ("% allowed in OpenMP only in %"
> >> " clauses");
> > 
> > ;-) Hmm, reminds me of the unresolved task to support the C++ "this"
> > pointer in OpenACC...  Anyway, in GCC trunk, we're not allowing "this"
> > usage, I think, so I suppose this should stay as-is?  (Possibly with an
> > OpenACC-specific error message.)
> 
> What do you want to do about c++'s 'this' in OpenACC? It looks like
> gomp4 partially supports it. Maybe we should wait to get clarification
> from the technical committee?

In OpenMP 4.5 it is only allowed in declare simd clauses, but that might
change in OpenMP 5.0.

Jakub

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-06-01 Thread Cesar Philippidis

On 05/25/2016 09:33 AM, Thomas Schwinge wrote:

>> --- gcc/c/c-parser.c
>> +++ gcc/c/c-parser.c
>> @@ -13602,6 +13602,7 @@ c_parser_oacc_declare (c_parser *parser)
>>  
>>switch (OMP_CLAUSE_MAP_KIND (t))
>>  {
>> +case GOMP_MAP_FIRSTPRIVATE_POINTER:
>>  case GOMP_MAP_FORCE_ALLOC:
>>  case GOMP_MAP_FORCE_TO:
>>  case GOMP_MAP_FORCE_DEVICEPTR:
> | case GOMP_MAP_DEVICE_RESIDENT:
> |   break;
> | 
> | case GOMP_MAP_POINTER:
> |   /* Generated by c_finish_omp_clauses from array sections;
> |  avoid spurious diagnostics.  */
> |   break;
> 
> Is "case GOMP_MAP_FIRSTPRIVATE_POINTER" meant to replace the "case
> GOMP_MAP_POINTER"?  If yes, then please remove that one (does that become
> gcc_unreachable?), and update/move the comment, or if not, please update
> the comment, too.  ;-)

This can be pruned out. I'm preparing a follow up patch with it's removal.

>> --- gcc/c/c-typeck.c
>> +++ gcc/c/c-typeck.c
> 
>>  /* Handle array sections for clause C.  */
>>  
>>  static bool
>> -handle_omp_array_sections (tree c, bool is_omp)
>> +handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>>  {
>>[...]
>> @@ -12427,7 +12427,7 @@ handle_omp_array_sections (tree c, bool is_omp)
>>&& TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE))
>>  return false;
>>gcc_assert (OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_FORCE_DEVICEPTR);
>> -  if (is_omp)
>> +  if (ort == C_ORT_OMP || ort == C_ORT_ACC)
>>  switch (OMP_CLAUSE_MAP_KIND (c))
>>{
>>case GOMP_MAP_ALLOC:
> |   case GOMP_MAP_TO:
> |   case GOMP_MAP_FROM:
> |   case GOMP_MAP_TOFROM:
> |   case GOMP_MAP_ALWAYS_TO:
> |   case GOMP_MAP_ALWAYS_FROM:
> |   case GOMP_MAP_ALWAYS_TOFROM:
> |   case GOMP_MAP_RELEASE:
> |   case GOMP_MAP_DELETE:
> | OMP_CLAUSE_MAP_MAYBE_ZERO_LENGTH_ARRAY_SECTION (c) = 1;
> | break;
> |   default:
>>  break;
>>}
> 
> Why doesn't that apply also to the other (OpenACC) map kinds?  Comparing
> to the full list in include/gomp-constants.h:enum gomp_map_kind, there
> are several missing here.

It does look like there are situations where OpenACC can take
zero-length arrays, e.g. when the length argument is a variable. This
will be fixed in my follow up patch.

>>tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
>> -  if (!is_omp)
>> +  if (ort != C_ORT_OMP && ort != C_ORT_ACC)
>>  OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
>>else if (TREE_CODE (t) == COMPONENT_REF)
>>  OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);
> 
>> --- gcc/cp/parser.c
>> +++ gcc/cp/parser.c
>> @@ -35214,6 +35214,7 @@ cp_parser_oacc_declare (cp_parser *parser, cp_token 
>> *pragma_tok)
>>gcc_assert (OMP_CLAUSE_CODE (t) == OMP_CLAUSE_MAP);
>>switch (OMP_CLAUSE_MAP_KIND (t))
>>  {
>> +case GOMP_MAP_FIRSTPRIVATE_POINTER:
>>  case GOMP_MAP_FORCE_ALLOC:
>>  case GOMP_MAP_FORCE_TO:
>>  case GOMP_MAP_FORCE_DEVICEPTR:
> 
> Likewise to my gcc/c/c-parser.c comments.
> 
>> --- gcc/cp/semantics.c
>> +++ gcc/cp/semantics.c
> 
>>  /* Handle array sections for clause C.  */
>>  
>>  static bool
>> -handle_omp_array_sections (tree c, bool is_omp)
>> +handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>>  {
>>[...]
>> @@ -4988,7 +4989,7 @@ handle_omp_array_sections (tree c, bool is_omp)
>>|| (TREE_CODE (t) == COMPONENT_REF
>>&& TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE))
>>  return false;
>> -  if (is_omp)
>> +  if (ort == C_ORT_OMP || ort == C_ORT_ACC)
>>  switch (OMP_CLAUSE_MAP_KIND (c))
>>{
>>case GOMP_MAP_ALLOC:
> 
> Likewise to my gcc/c/c-typeck.c comments.
> 
>> @@ -5007,7 +5008,7 @@ handle_omp_array_sections (tree c, bool is_omp)
>>}
>>tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
>>OMP_CLAUSE_MAP);
>> -  if (!is_omp)
>> +  if ((ort & C_ORT_OMP_DECLARE_SIMD) != C_ORT_OMP && ort != C_ORT_ACC)
>>  OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
>>else if (TREE_CODE (t) == COMPONENT_REF)
>>  OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);
> 
> Shouldn't that simply be "ort != C_ORT_OMP && ort != C_ORT_ACC"?

No, because then that wouldn't cover omp declare simd.

>> @@ -6054,7 +6070,7 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>  omp_note_field_privatization (t, OMP_CLAUSE_DECL (c));
>>else
>>  t = OMP_CLAUSE_DECL (c);
>> -  if (t == current_class_ptr)
>> +  if (ort != C_ORT_ACC && t == current_class_ptr)
>>  {
>>error ("% allowed in OpenMP only in %"
>>   " clauses");
> 
> ;-) Hmm, reminds me of the unresolved task to support the C++ "this"
> pointer in OpenACC...  Anyway, in GCC trunk, we're not allowing "this"
> usage, I think, so I suppose this s

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-25 Thread Thomas Schwinge

Hi!

A few more comments on the patch, as committed in r236678, also for
Chung-Lin and Tom.

The ChangeLos are missing references to GCC PRs, so these now should be
updated manually.  For example, your changes relate to PR70688 "bogus
OpenACC data clause errors involving reductions", and some of the
gcc/c/c-parser.c:c_finish_omp_clauses and
gcc/cp/parser.c:finish_omp_clauses changes (for OpenACC: "data clauses"
instead of "map clauses") and corresponding
gcc/testsuite/c-c++-common/goacc/data-clause-duplicate-1.c etc. test
suite updates relate to  "Adapt OpenMP
diagnostic messages for OpenACC" (but that still is to remain open until
addressed in full).  Don't know if there are any other related PRs?

> --- gcc/c/c-parser.c
> +++ gcc/c/c-parser.c
> @@ -13602,6 +13602,7 @@ c_parser_oacc_declare (c_parser *parser)
>  
>switch (OMP_CLAUSE_MAP_KIND (t))
>   {
> + case GOMP_MAP_FIRSTPRIVATE_POINTER:
>   case GOMP_MAP_FORCE_ALLOC:
>   case GOMP_MAP_FORCE_TO:
>   case GOMP_MAP_FORCE_DEVICEPTR:
|   case GOMP_MAP_DEVICE_RESIDENT:
| break;
| 
|   case GOMP_MAP_POINTER:
| /* Generated by c_finish_omp_clauses from array sections;
|avoid spurious diagnostics.  */
| break;

Is "case GOMP_MAP_FIRSTPRIVATE_POINTER" meant to replace the "case
GOMP_MAP_POINTER"?  If yes, then please remove that one (does that become
gcc_unreachable?), and update/move the comment, or if not, please update
the comment, too.  ;-)

> --- gcc/c/c-typeck.c
> +++ gcc/c/c-typeck.c

>  /* Handle array sections for clause C.  */
>  
>  static bool
> -handle_omp_array_sections (tree c, bool is_omp)
> +handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>  {
>[...]
> @@ -12427,7 +12427,7 @@ handle_omp_array_sections (tree c, bool is_omp)
> && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE))
>   return false;
>gcc_assert (OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_FORCE_DEVICEPTR);
> -  if (is_omp)
> +  if (ort == C_ORT_OMP || ort == C_ORT_ACC)
>   switch (OMP_CLAUSE_MAP_KIND (c))
> {
> case GOMP_MAP_ALLOC:
| case GOMP_MAP_TO:
| case GOMP_MAP_FROM:
| case GOMP_MAP_TOFROM:
| case GOMP_MAP_ALWAYS_TO:
| case GOMP_MAP_ALWAYS_FROM:
| case GOMP_MAP_ALWAYS_TOFROM:
| case GOMP_MAP_RELEASE:
| case GOMP_MAP_DELETE:
|   OMP_CLAUSE_MAP_MAYBE_ZERO_LENGTH_ARRAY_SECTION (c) = 1;
|   break;
| default:
>   break;
> }

Why doesn't that apply also to the other (OpenACC) map kinds?  Comparing
to the full list in include/gomp-constants.h:enum gomp_map_kind, there
are several missing here.

>tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
> -  if (!is_omp)
> +  if (ort != C_ORT_OMP && ort != C_ORT_ACC)
>   OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
>else if (TREE_CODE (t) == COMPONENT_REF)
>   OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);

> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -35214,6 +35214,7 @@ cp_parser_oacc_declare (cp_parser *parser, cp_token 
> *pragma_tok)
>gcc_assert (OMP_CLAUSE_CODE (t) == OMP_CLAUSE_MAP);
>switch (OMP_CLAUSE_MAP_KIND (t))
>   {
> + case GOMP_MAP_FIRSTPRIVATE_POINTER:
>   case GOMP_MAP_FORCE_ALLOC:
>   case GOMP_MAP_FORCE_TO:
>   case GOMP_MAP_FORCE_DEVICEPTR:

Likewise to my gcc/c/c-parser.c comments.

> --- gcc/cp/semantics.c
> +++ gcc/cp/semantics.c

>  /* Handle array sections for clause C.  */
>  
>  static bool
> -handle_omp_array_sections (tree c, bool is_omp)
> +handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>  {
>[...]
> @@ -4988,7 +4989,7 @@ handle_omp_array_sections (tree c, bool is_omp)
> || (TREE_CODE (t) == COMPONENT_REF
> && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE))
>   return false;
> -   if (is_omp)
> +   if (ort == C_ORT_OMP || ort == C_ORT_ACC)
>   switch (OMP_CLAUSE_MAP_KIND (c))
> {
> case GOMP_MAP_ALLOC:

Likewise to my gcc/c/c-typeck.c comments.

> @@ -5007,7 +5008,7 @@ handle_omp_array_sections (tree c, bool is_omp)
> }
> tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
> OMP_CLAUSE_MAP);
> -   if (!is_omp)
> +   if ((ort & C_ORT_OMP_DECLARE_SIMD) != C_ORT_OMP && ort != C_ORT_ACC)
>   OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
> else if (TREE_CODE (t) == COMPONENT_REF)
>   OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);

Shouldn't that simply be "ort != C_ORT_OMP && ort != C_ORT_ACC"?

> @@ -6054,7 +6070,7 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
>   omp_note_field_privatization (t, OMP_CLAUSE_DECL (c));
> else
>   t = OMP_CLAUSE_DECL (c);
> -   if (t == current_class_ptr)
> +   if (ort != C_ORT_ACC && t ==

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-24 Thread Jakub Jelinek

On Tue, May 24, 2016 at 12:16:35PM -0700, Cesar Philippidis wrote:
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -11939,8 +11939,7 @@ c_finish_omp_cancellation_point (location_t loc, tree 
> clauses)
>  
>  static tree
>  handle_omp_array_sections_1 (tree c, tree t, vec &types,
> -  bool &maybe_zero_len, unsigned int &first_non_one,
> -  bool is_omp)
> +  bool &maybe_zero_len, unsigned int &first_non_one)
>  {
>tree ret, low_bound, length, type;
>if (TREE_CODE (t) != TREE_LIST)
> @@ -11949,7 +11948,6 @@ handle_omp_array_sections_1 (tree c, tree t, 
> vec &types,
>   return error_mark_node;
>ret = t;
>if (TREE_CODE (t) == COMPONENT_REF
> -   && is_omp

Sorry, I've missed this one.  The patch is ok if you add on top of the
current patch ort argument to c-typeck.c (handle_omp_array_sections{,_1})
and use here && ort == C_ORT_OMP like in the C++ FE.

Jakub

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-24 Thread Cesar Philippidis

On 05/23/2016 11:09 PM, Jakub Jelinek wrote:
> On Mon, May 23, 2016 at 07:31:53PM -0700, Cesar Philippidis wrote:
>> @@ -12559,7 +12560,7 @@ c_finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>t = OMP_CLAUSE_DECL (c);
>>if (TREE_CODE (t) == TREE_LIST)
>>  {
>> -  if (handle_omp_array_sections (c, ort & C_ORT_OMP))
>> +  if (handle_omp_array_sections (c, ort & (C_ORT_OMP | C_ORT_ACC)))
>>  {
>>remove = true;
>>break;
> 
> You haven't touched the /c/ handle_omp_array_sections{,_1}.  As I said, I 
> believe
> you can just drop the is_omp argument altogether (unlike C++), or, pass for
> consistency ort itself there as well.  But I bet the argument will be
> unused.

OK, I removed is_omp. I only had to guard one call to
handle_omp_array_sections from c_finish_omp_clauses because OpenACC
doesn't support array reductions.

Is this OK for trunk?

Cesar

2016-05-24  Cesar Philippidis  

	gcc/c
	* c-parser.c (c_parser_oacc_declare): Add support for
	GOMP_MAP_FIRSTPRIVATE_POINTER.
	* c-typeck.c (handle_omp_array_sections_1): Remove is_omp argument.
	(handle_omp_array_sections): Likewise.
	(c_finish_omp_clauses): Add specific errors and warning messages for
	OpenACC.  Use firsrtprivate pointers for OpenACC subarrays.  Update
	calls to handle_omp_array_sections.

	gcc/cp/
	* parser.c (cp_parser_oacc_declare): Add support for
	GOMP_MAP_FIRSTPRIVATE_POINTER.
	* semantics.c (handle_omp_array_sections_1): Replace bool is_omp
	argument with enum c_omp_region_type ort.  Don't privatize OpenACC
	non-static members.
	(handle_omp_array_sections): Replace bool is_omp argument with enum
	c_omp_region_type ort.  Update call to handle_omp_array_sections_1.
	(finish_omp_clauses): Add specific errors and warning messages for
	OpenACC.  Use firsrtprivate pointers for OpenACC subarrays.  Update
	call to handle_omp_array_sections.

	gcc/
	* gimplify.c (omp_notice_variable): Use zero-length arrays for data
	pointers inside OACC_DATA regions.
	(gimplify_scan_omp_clauses): Prune firstprivate clause associated
	with OACC_DATA, OACC_ENTER_DATA and OACC_EXIT data regions.
	(gimplify_adjust_omp_clauses): Fix typo in comment.

	gcc/testsuite/
	* c-c++-common/goacc/data-clause-duplicate-1.c: Adjust test.
	* c-c++-common/goacc/deviceptr-1.c: Likewise.
	* c-c++-common/goacc/kernels-alias-3.c: Likewise.
	* c-c++-common/goacc/kernels-alias-4.c: Likewise.
	* c-c++-common/goacc/kernels-alias-5.c: Likewise.
	* c-c++-common/goacc/kernels-alias-8.c: Likewise.
	* c-c++-common/goacc/kernels-alias-ipa-pta-3.c: Likewise.
	* c-c++-common/goacc/pcopy.c: Likewise.
	* c-c++-common/goacc/pcopyin.c: Likewise.
	* c-c++-common/goacc/pcopyout.c: Likewise.
	* c-c++-common/goacc/pcreate.c: Likewise.
	* c-c++-common/goacc/pr70688.c: New test.
	* c-c++-common/goacc/present-1.c: Adjust test.
	* c-c++-common/goacc/reduction-5.c: Likewise.
	* g++.dg/goacc/data-1.C: New test.

	libgomp/
	* oacc-mem.c (acc_malloc): Update handling of shared-memory targets.
	(acc_free): Likewise.
	(acc_memcpy_to_device): Likewise.
	(acc_memcpy_from_device): Likewise.
	(acc_deviceptr): Likewise.
	(acc_hostptr): Likewise.
	(acc_is_present): Likewise.
	(acc_map_data): Likewise.
	(acc_unmap_data): Likewise.
	(present_create_copy): Likewise.
	(delete_copyout): Likewise.
	(update_dev_host): Likewise.
	* testsuite/libgomp.oacc-c-c++-common/asyncwait-1.c: Remove xfail.
	* testsuite/libgomp.oacc-c-c++-common/data-2-lib.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/data-2.c: Adjust test.
	* testsuite/libgomp.oacc-c-c++-common/data-3.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/enter_exit-lib.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/lib-13.c: Adjust test so that
	it only runs on nvptx targets.
	* testsuite/libgomp.oacc-c-c++-common/lib-14.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-15.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-16.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-17.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-18.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-20.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-21.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-22.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-23.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-24.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-25.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-28.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-29.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-30.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-34.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-42.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-43.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-44.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-47.c: Likewise.
	* testsuite/libgomp.oacc-c-c++-common/lib-48.c: Likewise.
	* testsuite/libgomp.oacc-c-c+

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-23 Thread Jakub Jelinek

On Mon, May 23, 2016 at 07:31:53PM -0700, Cesar Philippidis wrote:
> @@ -12559,7 +12560,7 @@ c_finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
> t = OMP_CLAUSE_DECL (c);
> if (TREE_CODE (t) == TREE_LIST)
>   {
> -   if (handle_omp_array_sections (c, ort & C_ORT_OMP))
> +   if (handle_omp_array_sections (c, ort & (C_ORT_OMP | C_ORT_ACC)))
>   {
> remove = true;
> break;

You haven't touched the /c/ handle_omp_array_sections{,_1}.  As I said, I 
believe
you can just drop the is_omp argument altogether (unlike C++), or, pass for
consistency ort itself there as well.  But I bet the argument will be
unused.

> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -4472,7 +4472,7 @@ omp_privatize_field (tree t, bool shared)
>  static tree
>  handle_omp_array_sections_1 (tree c, tree t, vec &types,
>bool &maybe_zero_len, unsigned int &first_non_one,
> -  bool is_omp)
> +  enum c_omp_region_type ort)
>  {
>tree ret, low_bound, length, type;
>if (TREE_CODE (t) != TREE_LIST)
> @@ -4484,7 +4484,7 @@ handle_omp_array_sections_1 (tree c, tree t, vec 
> &types,
>   t = TREE_OPERAND (t, 0);
>ret = t;
>if (TREE_CODE (t) == COMPONENT_REF
> -   && is_omp
> +   && ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP || ort == C_ORT_ACC)
> && (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_TO
> || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FROM)

This hunk looks wrong, I'd expect just && ort == C_ORT_OMP.
OpenACC to my knowledge doesn't have mapping of fields,
at least you reject copy(var.field) so it would surprise me
if you wanted to allow copy(var.field[:]).
And #pragma omp declare simd doesn't have any clauses for which it would
call handle_omp_array_sections* at all.

> @@ -4545,11 +4545,12 @@ handle_omp_array_sections_1 (tree c, tree t, 
> vec &types,
>return ret;
>  }
>  
> -  if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION
> +  if ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP
> +  && OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION

ort == C_ORT_OMP is enough.

> @@ -4988,7 +4989,7 @@ handle_omp_array_sections (tree c, bool is_omp)
> || (TREE_CODE (t) == COMPONENT_REF
> && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE))
>   return false;
> -   if (is_omp)
> +   if ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP || ort == C_ORT_ACC)

if (ort == C_ORT_OMP || ort == C_ORT_ACC)

Jakub

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-23 Thread Cesar Philippidis

On 05/20/2016 02:42 AM, Jakub Jelinek wrote:
> On Tue, May 10, 2016 at 01:29:50PM -0700, Cesar Philippidis wrote:

>> @@ -5796,12 +5796,14 @@ tree
>>  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>  {
>>bitmap_head generic_head, firstprivate_head, lastprivate_head;
>> -  bitmap_head aligned_head, map_head, map_field_head;
>> +  bitmap_head aligned_head, map_head, map_field_head, oacc_reduction_head;
>>tree c, t, *pc;
>>tree safelen = NULL_TREE;
>>bool branch_seen = false;
>>bool copyprivate_seen = false;
>>bool ordered_seen = false;
>> +  bool allow_fields = (ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP
>> +|| ort == C_ORT_ACC;
>>  
> 
> Formatting.  You want = already on the new line, or add () around the whole
> rhs and align || below (ort &.
> 
> Though, this looks wrong to me, does OpenACC all of sudden support
> privatization of non-static data members in methods?
> 
>>bitmap_obstack_initialize (NULL);
>>bitmap_initialize (&generic_head, &bitmap_default_obstack);
>> @@ -5810,6 +5812,7 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>bitmap_initialize (&aligned_head, &bitmap_default_obstack);
>>bitmap_initialize (&map_head, &bitmap_default_obstack);
>>bitmap_initialize (&map_field_head, &bitmap_default_obstack);
>> +  bitmap_initialize (&oacc_reduction_head, &bitmap_default_obstack);
>>  
>>for (pc = &clauses, c = clauses; c ; c = *pc)
>>  {
>> @@ -5829,8 +5832,7 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>t = OMP_CLAUSE_DECL (c);
>>if (TREE_CODE (t) == TREE_LIST)
>>  {
>> -  if (handle_omp_array_sections (c, ((ort & C_ORT_OMP_DECLARE_SIMD)
>> - == C_ORT_OMP)))
>> +  if (handle_omp_array_sections (c, allow_fields))
> 
> IMNSHO you don't want to change this, instead adjust C++
> handle_omp_array_sections* where it deals with array sections to just use
> the is_omp variant; there are still other places where it deals with
> non-static data members and I think you don't want to change those.

That should be fixed now. It looks like I only needed to prevent
handle_omp_array_sections_1 from calling omp_privatize_field for acc
regions. So I modified handle_omp_array_sections* to take a
c_omp_region_type argument instead of a bool is_omp to enable that.

finish_omp_clauses should be ok because it already has a field_ok
variable to guard that calls to omp_privatize_field.

>>  {
>>remove = true;
>>break;
>> @@ -6040,6 +6042,17 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>> omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
>>remove = true;
>>  }
>> +  else if (ort == C_ORT_ACC
>> +   && OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION)
>> +{
>> +  if (bitmap_bit_p (&oacc_reduction_head, DECL_UID (t)))
>> +{
>> +  error ("%qD appears more than once in reduction clauses", t);
>> +  remove = true;
>> +}
>> +  else
>> +bitmap_set_bit (&oacc_reduction_head, DECL_UID (t));
>> +}
>>else if (bitmap_bit_p (&generic_head, DECL_UID (t))
>> || bitmap_bit_p (&firstprivate_head, DECL_UID (t))
>> || bitmap_bit_p (&lastprivate_head, DECL_UID (t)))
>> @@ -6050,7 +6063,10 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE
>> && bitmap_bit_p (&map_head, DECL_UID (t)))
>>  {
>> -  error ("%qD appears both in data and map clauses", t);
>> +  if (ort == C_ORT_ACC)
>> +error ("%qD appears more than once in data clauses", t);
>> +  else
>> +error ("%qD appears both in data and map clauses", t);
>>remove = true;
>>  }
>>else
>> @@ -6076,7 +6092,7 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>  omp_note_field_privatization (t, OMP_CLAUSE_DECL (c));
>>else
>>  t = OMP_CLAUSE_DECL (c);
>> -  if (t == current_class_ptr)
>> +  if (ort != C_ORT_ACC && t == current_class_ptr)
>>  {
>>error ("% allowed in OpenMP only in %"
>>   " clauses");
>> @@ -6103,7 +6119,10 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>  }
>>else if (bitmap_bit_p (&map_head, DECL_UID (t)))
>>  {
>> -  error ("%qD appears both in data and map clauses", t);
>> +  if (ort == C_ORT_ACC)
>> +error ("%qD appears more than once in data clauses", t);
>> +  else
>> +error ("%qD appears both in data and map clauses", t);
>>remove = true;
>>  }
>>else
>> @@ -6551,8 +6570,7 @@ finish_omp_clauses (tree clauses, enum 
>> c_omp_region_type ort)
>>

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-20 Thread Jakub Jelinek

On Tue, May 10, 2016 at 01:29:50PM -0700, Cesar Philippidis wrote:
> @@ -12542,7 +12543,7 @@ c_finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
> t = OMP_CLAUSE_DECL (c);
> if (TREE_CODE (t) == TREE_LIST)
>   {
> -   if (handle_omp_array_sections (c, ort & C_ORT_OMP))
> +   if (handle_omp_array_sections (c, ort & (C_ORT_OMP | C_ORT_ACC)))
>   {
> remove = true;
> break;

There are no array sections in Cilk+, so the above is always then (for C).
Thus, the is_omp argument to handle_omp_array_sections* should be removed
and code adjusted.

> +   if (ort == C_ORT_ACC)
> + error ("%qD appears more than once in data clasues", t);

Typo.

> +   else
> + error ("%qD appears both in data and map clauses", t);
> remove = true;
>   }
> else
> @@ -12893,7 +12908,10 @@ c_finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
>   }
> else if (bitmap_bit_p (&map_head, DECL_UID (t)))
>   {
> -   error ("%qD appears both in data and map clauses", t);
> +   if (ort == C_ORT_ACC)
> + error ("%qD appears more than once in data clasues", t);

Likewise.

> +   else
> + error ("%qD appears both in data and map clauses", t);
> remove = true;
>   }
> else

> @@ -5796,12 +5796,14 @@ tree
>  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  {
>bitmap_head generic_head, firstprivate_head, lastprivate_head;
> -  bitmap_head aligned_head, map_head, map_field_head;
> +  bitmap_head aligned_head, map_head, map_field_head, oacc_reduction_head;
>tree c, t, *pc;
>tree safelen = NULL_TREE;
>bool branch_seen = false;
>bool copyprivate_seen = false;
>bool ordered_seen = false;
> +  bool allow_fields = (ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP
> +|| ort == C_ORT_ACC;
>  

Formatting.  You want = already on the new line, or add () around the whole
rhs and align || below (ort &.

Though, this looks wrong to me, does OpenACC all of sudden support
privatization of non-static data members in methods?

>bitmap_obstack_initialize (NULL);
>bitmap_initialize (&generic_head, &bitmap_default_obstack);
> @@ -5810,6 +5812,7 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
>bitmap_initialize (&aligned_head, &bitmap_default_obstack);
>bitmap_initialize (&map_head, &bitmap_default_obstack);
>bitmap_initialize (&map_field_head, &bitmap_default_obstack);
> +  bitmap_initialize (&oacc_reduction_head, &bitmap_default_obstack);
>  
>for (pc = &clauses, c = clauses; c ; c = *pc)
>  {
> @@ -5829,8 +5832,7 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
> t = OMP_CLAUSE_DECL (c);
> if (TREE_CODE (t) == TREE_LIST)
>   {
> -   if (handle_omp_array_sections (c, ((ort & C_ORT_OMP_DECLARE_SIMD)
> -  == C_ORT_OMP)))
> +   if (handle_omp_array_sections (c, allow_fields))

IMNSHO you don't want to change this, instead adjust C++
handle_omp_array_sections* where it deals with array sections to just use
the is_omp variant; there are still other places where it deals with
non-static data members and I think you don't want to change those.
>   {
> remove = true;
> break;
> @@ -6040,6 +6042,17 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
>  omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
> remove = true;
>   }
> +   else if (ort == C_ORT_ACC
> +&& OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION)
> + {
> +   if (bitmap_bit_p (&oacc_reduction_head, DECL_UID (t)))
> + {
> +   error ("%qD appears more than once in reduction clauses", t);
> +   remove = true;
> + }
> +   else
> + bitmap_set_bit (&oacc_reduction_head, DECL_UID (t));
> + }
> else if (bitmap_bit_p (&generic_head, DECL_UID (t))
>  || bitmap_bit_p (&firstprivate_head, DECL_UID (t))
>  || bitmap_bit_p (&lastprivate_head, DECL_UID (t)))
> @@ -6050,7 +6063,10 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
> else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE
>  && bitmap_bit_p (&map_head, DECL_UID (t)))
>   {
> -   error ("%qD appears both in data and map clauses", t);
> +   if (ort == C_ORT_ACC)
> + error ("%qD appears more than once in data clauses", t);
> +   else
> + error ("%qD appears both in data and map clauses", t);
> remove = true;
>   }
> else
> @@ -6076,7 +6092,7 @@ finish_omp_clauses (tree clauses, enum 
> c_omp_region_type ort)
>   omp_note_field_privatization (t, OMP_

Re: [ping][patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-19 Thread Cesar Philippidis

Ping.

Cesar

On 05/10/2016 01:29 PM, Cesar Philippidis wrote:
> Pointers are special in OpenACC. Depending on the context, they can
> either be treated as a "scalar" or as special firstprivate pointer. This
> is in contrast to OpenMP target pointers, which are always treated as
> firstprivate pointers if I'm not mistaken. The difference between a
> firstprivate scalar and pointer is that the contents of a firstprivate
> scalar are preserved on the accelerator, whereas a firstprivate pointer
> gets remapped by the runtime to point to an address in the device's
> address space. Here are the rules for pointers that I worked out with
> the ACC technical committee.
> 
>   1) pointers used in subarrays shall be treated as firstprivate
>  pointers
> 
>   2) all other pointers are scalars
> 
> There is an exception to 2) when a pointer appears inside a data region.
> E.g.
> 
>   #pragma acc data copy (ptr[0:100])
>   {
> #pragma acc parallel loop
> for (i = ...)
>   ptr[i] = ...
>   }
> 
> Here the compiler should detect that ptr is nested inside an acc data
> region, and add an implicit present(ptr[0:100]) clause to it, and not
> present(ptr). Note that the implicit data clause rule only applies to
> lexically scoped offloaded regions inside acc data regions. E.g.
> 
>   foo (int *ptr)
>   {
> ...
> #pragma acc parallel loop
> for (i = ...)
>ptr[i] = ...
>   }
> 
>   bar ()
>   {
> ...
> #pragma acc data copy (ptr[0:100])
> {
>   foo (ptr);
> }
>   }
> 
> will result in an implicit firstprivate(ptr) clause of the scalar
> variety, not a firstprivate_pointer(ptr).
> 
> The attached patch updates gcc to implement this behavior. Currently,
> gcc treats all pointers as scalars in OpenACC. So, if you have a
> subarray involving a data mapping pcopy(p[5:10]), the gimplifier would
> translate this clause into
> 
>   map(tofrom:*(p + 3) [len: 5]) map(alloc:p [pointer assign, bias: 3])
> 
> The alloc pointer map is a problem, especially with subarrays, because
> it effectively breaks all of the acc_* library functions involving
> subarrays. This patch changes that alloc map clause into a
> map(firstprivate:c [pointer assign, bias: 3]).
> 
> This patch also corrects behavior of the acc_* library functions when
> they deal with shared-memory targets. Before, most of those libraries
> were incorrectly trying to create data maps for shared-memory targets.
> The new behavior is to propagate the the host pointers where applicable,
> bypassing the data map altogether.
> 
> Since I had to change so many existing compiler test cases, I also took
> the liberty to make some of warning and error messages generated by the
> c and c++ front ends more specific to OpenACC. In the c++ front end, I
> went one step further and added preliminary checking for duplicate
> reference-typed data mappings. This check is not that exhaustive though,
> but I did include a test case for OpenMP.
> 
> It should be noted that I still need to update the behavior of subarray
> pointers in fortran. I'm just waiting until for the OpenMP 4.5 fortran
> changes to land in trunk first.
> 
> Is this patch OK for trunk?
> 
> Cesar
>

[patch,openacc] use firstprivate pointers for subarrays in c and c++

2016-05-10 Thread Cesar Philippidis

Pointers are special in OpenACC. Depending on the context, they can
either be treated as a "scalar" or as special firstprivate pointer. This
is in contrast to OpenMP target pointers, which are always treated as
firstprivate pointers if I'm not mistaken. The difference between a
firstprivate scalar and pointer is that the contents of a firstprivate
scalar are preserved on the accelerator, whereas a firstprivate pointer
gets remapped by the runtime to point to an address in the device's
address space. Here are the rules for pointers that I worked out with
the ACC technical committee.

  1) pointers used in subarrays shall be treated as firstprivate
 pointers

  2) all other pointers are scalars

There is an exception to 2) when a pointer appears inside a data region.
E.g.

  #pragma acc data copy (ptr[0:100])
  {
#pragma acc parallel loop
for (i = ...)
  ptr[i] = ...
  }

Here the compiler should detect that ptr is nested inside an acc data
region, and add an implicit present(ptr[0:100]) clause to it, and not
present(ptr). Note that the implicit data clause rule only applies to
lexically scoped offloaded regions inside acc data regions. E.g.

  foo (int *ptr)
  {
...
#pragma acc parallel loop
for (i = ...)
   ptr[i] = ...
  }

  bar ()
  {
...
#pragma acc data copy (ptr[0:100])
{
  foo (ptr);
}
  }

will result in an implicit firstprivate(ptr) clause of the scalar
variety, not a firstprivate_pointer(ptr).

The attached patch updates gcc to implement this behavior. Currently,
gcc treats all pointers as scalars in OpenACC. So, if you have a
subarray involving a data mapping pcopy(p[5:10]), the gimplifier would
translate this clause into

  map(tofrom:*(p + 3) [len: 5]) map(alloc:p [pointer assign, bias: 3])

The alloc pointer map is a problem, especially with subarrays, because
it effectively breaks all of the acc_* library functions involving
subarrays. This patch changes that alloc map clause into a
map(firstprivate:c [pointer assign, bias: 3]).

This patch also corrects behavior of the acc_* library functions when
they deal with shared-memory targets. Before, most of those libraries
were incorrectly trying to create data maps for shared-memory targets.
The new behavior is to propagate the the host pointers where applicable,
bypassing the data map altogether.

Since I had to change so many existing compiler test cases, I also took
the liberty to make some of warning and error messages generated by the
c and c++ front ends more specific to OpenACC. In the c++ front end, I
went one step further and added preliminary checking for duplicate
reference-typed data mappings. This check is not that exhaustive though,
but I did include a test case for OpenMP.

It should be noted that I still need to update the behavior of subarray
pointers in fortran. I'm just waiting until for the OpenMP 4.5 fortran
changes to land in trunk first.

Is this patch OK for trunk?

Cesar

2016-05-10  Cesar Philippidis  

	gcc/c/
	* c-parser.c (c_parser_oacc_declare): Add support for
	GOMP_MAP_FIRSTPRIVATE_POINTER.
	* c-typeck.c (c_finish_omp_clauses): Add specific errors and warning
	messages for OpenACC.  Use firsrtprivate pointers for OpenACC subarrays.

	gcc/cp/
	* parser.c (cp_parser_oacc_declare): Add support for
	GOMP_MAP_FIRSTPRIVATE_POINTERS.
	* semantics.c (finish_omp_clauses): Add specific errors and warning
	messages for OpenACC.  Use firsrtprivate pointers for OpenACC subarrays.
	Add some checking for duplicate reference-typed data mappings.

	gcc/
	* gimplify.c (omp_notice_variable): Use zero-length arrays for data
	pointers inside OACC_DATA regions.
	(gimplify_scan_omp_clauses): Prune firstprivate clause associated
	with OACC_DATA, OACC_ENTER_DATA and OACC_EXIT data regions.

	gcc/testsuite/
	* c-c++-common/goacc/data-clause-duplicate-1.c: Adjust test.
	* c-c++-common/goacc/deviceptr-1.c: Likewise.
	* c-c++-common/goacc/kernels-alias-3.c: Likewise.
	* c-c++-common/goacc/kernels-alias-4.c: Likewise.
	* c-c++-common/goacc/kernels-alias-5.c: Likewise.
	* c-c++-common/goacc/kernels-alias-8.c: Likewise.
	* c-c++-common/goacc/kernels-alias-ipa-pta-3.c: Likewise.
	* c-c++-common/goacc/pcopy.c: Likewise.
	* c-c++-common/goacc/pcopyin.c: Likewise.
	* c-c++-common/goacc/pcopyout.c: Likewise.
	* c-c++-common/goacc/pcreate.c: Likewise.
	* c-c++-common/goacc/pr70688.c: New test.
	* c-c++-common/goacc/present-1.c: Adjust test.
	* c-c++-common/goacc/reduction-5.c: Likewise.
	* g++.dg/goacc/data-1.C: New test.
	* g++.dg/goacc/data-2.C: New test.
	* g++.dg/gomp/template-data.C: New test.

	libgomp/
	* oacc-mem.c (acc_malloc): Update handling of shared-memory targets.
	(acc_free): Likewise.
	(acc_memcpy_to_device): Likewise.
	(acc_memcpy_from_device): Likewise.
	(acc_deviceptr): Likewise.
	(acc_hostptr): Likewise.
	(acc_is_present): Likewise.
	(acc_map_data): Likewise.
	(acc_unmap_data): Likewise.
	(present_create_copy): Likewise.
	(delete_copyout): Likewise.
	(update_dev_host): Likewise.
	*

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [patch,openacc] use firstprivate pointers for subarrays in c and c++

Re: [ping][patch,openacc] use firstprivate pointers for subarrays in c and c++

[patch,openacc] use firstprivate pointers for subarrays in c and c++

10 matches

Site Navigation

Mail list logo

Footer information