public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
@ 2018-08-30 19:32 Julian Brown
  2018-08-30 20:27 ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Julian Brown @ 2018-08-30 19:32 UTC (permalink / raw)
  To: gcc-patches, joseph, jakub

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

This patch (by Joseph) allows "this" to be used in OpenACC directives,
following -- IIUC -- the behaviour of other compilers. The standard (as
of OpenACC 2.5) does not appear to have explicit language either
permitting or forbidding such usage.

Joseph's original commentary is in the bug report (ca. 2015,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66053):

"This patch, for gomp-4_0-branch, adds support for C++ "this" in
OpenACC directives.  (This patch does not do anything to handle OpenMP
differently from OpenACC; that - bug 66053 - will need to be resolved
for mainline, either deciding these cases should be accepted for
OpenMP or making the parsing only accept them in OpenACC directives
and not OpenMP ones.)

"Apart from parsing, it's necessary to prevent the "cannot take the
address of 'this', which is an rvalue expression" error from appearing
when "this" is used in such contexts.  This patch duly adds a new
argument to cxx_mark_addressable (default false so callers don't all
need to change) to allow disabling that error, passing that argument
in all calls that seem relevant to OpenACC directives."

AFAICT though, this attached version of the patch does still forbid
"this" in OpenMP directives (apart from "declare simd"). I couldn't see
that there was any change to the OpenMP spec language in 4.5.

Tested with offloading to NVPTX and bootstrapped.

OK to apply?

Julian

ChangeLog

20xx-xx-xx  Joseph Myers  <joseph@codesourcery.com>

        PR C++/66053

        gcc/cp/
        * cp-tree.h (enum cxx_mark_addressable_flags): New.
        (cxx_mark_addressable): Use it.  Adjust users.
        * parser.c (cp_parser_omp_var_list_no_open): Handle RID_THIS.
        * semantics.c (handle_omp_array_sections_1)
        (handle_omp_array_sections, finish_omp_reduction_clause)
        (finish_omp_clauses): Pass CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS
        to cxx_mark_addressable.  Enforce "this" usage limitation only
        for OpenMP.
        * typeck.c (cp_build_array_ref): Adjust cxx_mark_addressble
        call. (cxx_mark_addressable): Handle
        CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS.

        libgomp/
        * testsuite/libgomp.oacc-c++/this.C: New test.

[-- Attachment #2: this-in-openacc-directives-2.diff --]
[-- Type: text/x-patch, Size: 8238 bytes --]

commit 12294a1345d981b72ef61d285057fb4c7e378fd7
Author: Julian Brown <julian@codesourcery.com>
Date:   Wed Aug 29 18:19:44 2018 -0700

    Support C++ "this" in OpenACC directives
    
    20xx-xx-xx  Joseph Myers  <joseph@codesourcery.com>
    
    	PR C++/66053
    
    	gcc/cp/
    	* cp-tree.h (enum cxx_mark_addressable_flags): New.
    	(cxx_mark_addressable): Use it.  Adjust users.
    	* parser.c (cp_parser_omp_var_list_no_open): Handle RID_THIS.
    	* semantics.c (handle_omp_array_sections_1)
    	(handle_omp_array_sections, finish_omp_reduction_clause)
    	(finish_omp_clauses): Pass CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS
    	to cxx_mark_addressable.  Enforce "this" usage limitation only for
    	OpenMP.
    	* typeck.c (cp_build_array_ref): Adjust cxx_mark_addressble call.
    	(cxx_mark_addressable): Handle CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS.
    
    	libgomp/
    	* testsuite/libgomp.oacc-c++/this.C: New test.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 43e452c..127e15a 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7214,7 +7214,24 @@ extern void cxx_print_error_function		(diagnostic_context *,
 						 struct diagnostic_info *);
 
 /* in typeck.c */
-extern bool cxx_mark_addressable		(tree, bool = false);
+
+/* Flags for cxx_mark_addressable.  */
+
+enum cxx_mark_addressable_flags
+{
+  CXX_MARK_ADDRESSABLE_FLAGS_NONE = 0,
+  /* This is for ARRAY_REF construction - in that case we don't want
+     to look through VIEW_CONVERT_EXPR from VECTOR_TYPE to ARRAY_TYPE,
+     it is fine to use ARRAY_REFs for vector subscripts on vector
+     register variables.  */
+  CXX_MARK_ADDRESSABLE_FLAGS_ARRAY_REF = 1 << 0,
+  /* Allow `current_class_ptr' to be addressable.  */
+  CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS = 1 << 1
+};
+
+extern bool cxx_mark_addressable		(tree,
+						 enum cxx_mark_addressable_flags flags
+						 = CXX_MARK_ADDRESSABLE_FLAGS_NONE);
 extern int string_conv_p			(const_tree, const_tree, int);
 extern tree cp_truthvalue_conversion		(tree);
 extern tree condition_conversion		(tree);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 92e6b40..0cf2526 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -31726,7 +31726,7 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
 	  OMP_CLAUSE_CHAIN (u) = list;
 	  list = u;
 	}
-      else
+      else if (decl != error_mark_node)
 	list = tree_cons (decl, NULL_TREE, list);
 
     get_comma:
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 676de01..9a722df 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -4598,7 +4598,8 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree> &types,
 		      omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
 	  return error_mark_node;
 	}
-      else if (TREE_CODE (t) == PARM_DECL
+      else if (ort == C_ORT_OMP
+	       && TREE_CODE (t) == PARM_DECL
 	       && DECL_ARTIFICIAL (t)
 	       && DECL_NAME (t) == this_identifier)
 	{
@@ -5101,7 +5102,8 @@ handle_omp_array_sections (tree c, enum c_omp_region_type ort)
 	  else
 	    OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_FIRSTPRIVATE_POINTER);
 	  if (OMP_CLAUSE_MAP_KIND (c2) != GOMP_MAP_FIRSTPRIVATE_POINTER
-	      && !cxx_mark_addressable (t))
+	      && !cxx_mark_addressable (t,
+					CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS))
 	    return false;
 	  OMP_CLAUSE_DECL (c2) = t;
 	  t = build_fold_addr_expr (first);
@@ -5689,7 +5691,8 @@ finish_omp_reduction_clause (tree c, bool *need_default_ctor, bool *need_dtor)
 	      if (TREE_ADDRESSABLE (DECL_EXPR_DECL (stmts[1]))
 		  && !TYPE_REF_P (TREE_TYPE (OMP_CLAUSE_DECL (c))))
 		cxx_mark_addressable (decl_placeholder ? decl_placeholder
-				      : OMP_CLAUSE_DECL (c));
+				      : OMP_CLAUSE_DECL (c),
+				      CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS);
 	      tree omp_out = placeholder;
 	      tree omp_in = decl_placeholder ? decl_placeholder
 			    : convert_from_reference (OMP_CLAUSE_DECL (c));
@@ -5715,7 +5718,8 @@ finish_omp_reduction_clause (tree c, bool *need_default_ctor, bool *need_dtor)
 			  && TREE_CODE (stmts[4]) == DECL_EXPR);
 	      if (TREE_ADDRESSABLE (DECL_EXPR_DECL (stmts[3])))
 		cxx_mark_addressable (decl_placeholder ? decl_placeholder
-				      : OMP_CLAUSE_DECL (c));
+				      : OMP_CLAUSE_DECL (c),
+				      CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS);
 	      if (TREE_ADDRESSABLE (DECL_EXPR_DECL (stmts[4])))
 		cxx_mark_addressable (placeholder);
 	      tree omp_priv = decl_placeholder ? decl_placeholder
@@ -6655,7 +6659,8 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	      remove = true;
 	    }
 	  else if (!processing_template_decl
-		   && !cxx_mark_addressable (t))
+		   && !cxx_mark_addressable (t,
+					     CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS))
 	    remove = true;
 	  break;
 
@@ -6803,7 +6808,8 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 		   && (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP
 		       || (OMP_CLAUSE_MAP_KIND (c)
 			   != GOMP_MAP_FIRSTPRIVATE_POINTER))
-		   && !cxx_mark_addressable (t))
+		   && !cxx_mark_addressable (t,
+					     CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS))
 	    remove = true;
 	  else if (!(OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
 		     && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_POINTER
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index ab088a9..b24f3b2 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3335,7 +3335,8 @@ cp_build_array_ref (location_t loc, tree array, tree idx,
 	      && (TREE_CODE (TYPE_SIZE (TREE_TYPE (TREE_TYPE (array))))
 		  != INTEGER_CST)))
 	{
-	  if (!cxx_mark_addressable (array, true))
+	  if (!cxx_mark_addressable (array,
+				     CXX_MARK_ADDRESSABLE_FLAGS_ARRAY_REF))
 	    return error_mark_node;
 	}
 
@@ -6476,16 +6477,10 @@ unary_complex_lvalue (enum tree_code code, tree arg)
 \f
 /* Mark EXP saying that we need to be able to take the
    address of it; it should not be allocated in a register.
-   Value is true if successful.  ARRAY_REF_P is true if this
-   is for ARRAY_REF construction - in that case we don't want
-   to look through VIEW_CONVERT_EXPR from VECTOR_TYPE to ARRAY_TYPE,
-   it is fine to use ARRAY_REFs for vector subscripts on vector
-   register variables.
-
-   C++: we do not allow `current_class_ptr' to be addressable.  */
+   Value is true if successful.  */
 
 bool
-cxx_mark_addressable (tree exp, bool array_ref_p)
+cxx_mark_addressable (tree exp, enum cxx_mark_addressable_flags flags)
 {
   tree x = exp;
 
@@ -6493,7 +6488,7 @@ cxx_mark_addressable (tree exp, bool array_ref_p)
     switch (TREE_CODE (x))
       {
       case VIEW_CONVERT_EXPR:
-	if (array_ref_p
+	if ((flags & CXX_MARK_ADDRESSABLE_FLAGS_ARRAY_REF)
 	    && TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
 	    && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (x, 0))))
 	  return true;
@@ -6509,7 +6504,8 @@ cxx_mark_addressable (tree exp, bool array_ref_p)
       case PARM_DECL:
 	if (x == current_class_ptr)
 	  {
-	    error ("cannot take the address of %<this%>, which is an rvalue expression");
+	    if (!(flags & CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS))
+	      error ("cannot take the address of %<this%>, which is an rvalue expression");
 	    TREE_ADDRESSABLE (x) = 1; /* so compiler doesn't die later.  */
 	    return true;
 	  }
@@ -6552,7 +6548,7 @@ cxx_mark_addressable (tree exp, bool array_ref_p)
 
       case TARGET_EXPR:
 	TREE_ADDRESSABLE (x) = 1;
-	cxx_mark_addressable (TREE_OPERAND (x, 0));
+	cxx_mark_addressable (TREE_OPERAND (x, 0), flags);
 	return true;
 
       default:
diff --git a/libgomp/testsuite/libgomp.oacc-c++/this.C b/libgomp/testsuite/libgomp.oacc-c++/this.C
new file mode 100644
index 0000000..5e70f3d
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-c++/this.C
@@ -0,0 +1,43 @@
+#include <cstdlib>
+#include <iostream>
+using namespace std;
+
+class test {
+  public:
+  int a;
+
+  test ()
+  {
+    a = -1;
+#pragma acc enter data copyin (this[0:1])
+  }
+
+  ~test ()
+  {
+#pragma acc exit data delete (this[0:1])
+  }
+  
+  void set (int i)
+  {
+    a = i;
+#pragma acc update device (this[0:1])
+  }
+
+  int get ()
+  {
+#pragma acc update host (this[0:1])
+    return a;
+  }
+};
+
+int
+main ()
+{
+  test t;
+
+  t.set (4);
+  if (t.get () != 4)
+    abort ();
+
+  return 0;
+}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
  2018-08-30 19:32 [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053) Julian Brown
@ 2018-08-30 20:27 ` Jason Merrill
  2018-08-31 14:04   ` Nathan Sidwell
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2018-08-30 20:27 UTC (permalink / raw)
  To: Julian Brown; +Cc: gcc-patches List, Joseph S. Myers, Jakub Jelinek

On Thu, Aug 30, 2018 at 3:31 PM, Julian Brown <julian@codesourcery.com> wrote:
> This patch (by Joseph) allows "this" to be used in OpenACC directives,
> following -- IIUC -- the behaviour of other compilers. The standard (as
> of OpenACC 2.5) does not appear to have explicit language either
> permitting or forbidding such usage.
>
> Joseph's original commentary is in the bug report (ca. 2015,
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66053):
>
> "This patch, for gomp-4_0-branch, adds support for C++ "this" in
> OpenACC directives.  (This patch does not do anything to handle OpenMP
> differently from OpenACC; that - bug 66053 - will need to be resolved
> for mainline, either deciding these cases should be accepted for
> OpenMP or making the parsing only accept them in OpenACC directives
> and not OpenMP ones.)
>
> "Apart from parsing, it's necessary to prevent the "cannot take the
> address of 'this', which is an rvalue expression" error from appearing
> when "this" is used in such contexts.  This patch duly adds a new
> argument to cxx_mark_addressable (default false so callers don't all
> need to change) to allow disabling that error, passing that argument
> in all calls that seem relevant to OpenACC directives."
>
> AFAICT though, this attached version of the patch does still forbid
> "this" in OpenMP directives (apart from "declare simd"). I couldn't see
> that there was any change to the OpenMP spec language in 4.5.
>
> Tested with offloading to NVPTX and bootstrapped.
>
> OK to apply?
>
> Julian
>
> ChangeLog
>
> 20xx-xx-xx  Joseph Myers  <joseph@codesourcery.com>
>
>         PR C++/66053
>
>         gcc/cp/
>         * cp-tree.h (enum cxx_mark_addressable_flags): New.
>         (cxx_mark_addressable): Use it.  Adjust users.
>         * parser.c (cp_parser_omp_var_list_no_open): Handle RID_THIS.
>         * semantics.c (handle_omp_array_sections_1)
>         (handle_omp_array_sections, finish_omp_reduction_clause)
>         (finish_omp_clauses): Pass CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS
>         to cxx_mark_addressable.  Enforce "this" usage limitation only
>         for OpenMP.
>         * typeck.c (cp_build_array_ref): Adjust cxx_mark_addressble
>         call. (cxx_mark_addressable): Handle
>         CXX_MARK_ADDRESSABLE_FLAGS_ALLOW_THIS.
>
>         libgomp/
>         * testsuite/libgomp.oacc-c++/this.C: New test.

Why does referring to this[0:1] require making 'this' addressable?
Surely what we're interested in is the value of 'this', not the
address.

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
  2018-08-30 20:27 ` Jason Merrill
@ 2018-08-31 14:04   ` Nathan Sidwell
  2018-08-31 14:20     ` Jakub Jelinek
  0 siblings, 1 reply; 6+ messages in thread
From: Nathan Sidwell @ 2018-08-31 14:04 UTC (permalink / raw)
  To: Jason Merrill, Julian Brown
  Cc: gcc-patches List, Joseph S. Myers, Jakub Jelinek

On 08/30/2018 04:27 PM, Jason Merrill wrote:

> On Thu, Aug 30, 2018 at 3:31 PM, Julian Brown <julian@codesourcery.com> wrote:
>
>> "Apart from parsing, it's necessary to prevent the "cannot take the
>> address of 'this', which is an rvalue expression" error from appearing

Breaking a rather fundamental language attribute does not seem wise.

> Why does referring to this[0:1] require making 'this' addressable?
> Surely what we're interested in is the value of 'this', not the
> address.
Yes, transferring the this pointer is very unlikely to be what the user 
wants -- the object being referred to contains the data.  It might be 
wise to look at the DR's and changes relating to lambdas and this 
capture.  Those changes now make it much harder to simply capture the 
pointer unintentionally.

nathan
-- 
Nathan Sidwell

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
  2018-08-31 14:04   ` Nathan Sidwell
@ 2018-08-31 14:20     ` Jakub Jelinek
  2018-09-03 23:50       ` Julian Brown
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2018-08-31 14:20 UTC (permalink / raw)
  To: Nathan Sidwell
  Cc: Jason Merrill, Julian Brown, gcc-patches List, Joseph S. Myers

On Fri, Aug 31, 2018 at 10:04:07AM -0400, Nathan Sidwell wrote:
> On 08/30/2018 04:27 PM, Jason Merrill wrote:
> 
> > On Thu, Aug 30, 2018 at 3:31 PM, Julian Brown <julian@codesourcery.com> wrote:
> > 
> > > "Apart from parsing, it's necessary to prevent the "cannot take the
> > > address of 'this', which is an rvalue expression" error from appearing
> 
> Breaking a rather fundamental language attribute does not seem wise.
> 
> > Why does referring to this[0:1] require making 'this' addressable?
> > Surely what we're interested in is the value of 'this', not the
> > address.
> Yes, transferring the this pointer is very unlikely to be what the user
> wants -- the object being referred to contains the data.  It might be wise
> to look at the DR's and changes relating to lambdas and this capture.  Those
> changes now make it much harder to simply capture the pointer
> unintentionally.

Yeah, I agree we shouldn't try to make this addressable.
Does OpenACC try to map the base of the array section (rather than what e.g.
OpenMP does, privatize the pointer base instead and assign the pointer the
new value inside of the region)?
Even if it is mapped, can't it be mapped by taking an address of a temporary
initialized from this?

	Jakub

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
  2018-08-31 14:20     ` Jakub Jelinek
@ 2018-09-03 23:50       ` Julian Brown
  2018-10-25 20:08         ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Julian Brown @ 2018-09-03 23:50 UTC (permalink / raw)
  To: Jakub Jelinek
  Cc: Nathan Sidwell, Jason Merrill, gcc-patches List, Joseph S. Myers

[-- Attachment #1: Type: text/plain, Size: 3245 bytes --]

On Fri, 31 Aug 2018 16:20:08 +0200
Jakub Jelinek <jakub@redhat.com> wrote:

> On Fri, Aug 31, 2018 at 10:04:07AM -0400, Nathan Sidwell wrote:
> > On 08/30/2018 04:27 PM, Jason Merrill wrote:
> >   
> > > On Thu, Aug 30, 2018 at 3:31 PM, Julian Brown
> > > <julian@codesourcery.com> wrote: 
> > > > "Apart from parsing, it's necessary to prevent the "cannot take
> > > > the address of 'this', which is an rvalue expression" error
> > > > from appearing  
> > 
> > Breaking a rather fundamental language attribute does not seem wise.
> >   
> > > Why does referring to this[0:1] require making 'this' addressable?
> > > Surely what we're interested in is the value of 'this', not the
> > > address.  
> > Yes, transferring the this pointer is very unlikely to be what the
> > user wants -- the object being referred to contains the data.  It
> > might be wise to look at the DR's and changes relating to lambdas
> > and this capture.  Those changes now make it much harder to simply
> > capture the pointer unintentionally.  
> 
> Yeah, I agree we shouldn't try to make this addressable.
> Does OpenACC try to map the base of the array section (rather than
> what e.g. OpenMP does, privatize the pointer base instead and assign
> the pointer the new value inside of the region)?
> Even if it is mapped, can't it be mapped by taking an address of a
> temporary initialized from this?

For OpenACC, two mappings are created for an array section: one for the
data (to, from, tofrom, etc.) and a firstprivate pointer with a bias to
locate the (possibly virtual) zero'th element of the array. I think
that's the same as OpenMP.

For the test case given, it's sufficient to merely allow "this" to be
used as the base pointer for an array section. That usage doesn't
require "this" to be made addressable.

The this[0:1] syntax is accepted by PGI
(https://www.pgroup.com/resources/docs/18.4/x86/openacc-gs/index.htm,
2.4 C++ Classes in OpenACC) -- in order to copy "the class itself" to
the accelerator.

Referring to class member variables in OpenACC clauses (as the example
in that section does also) is still problematic in GCC, though.

PGI also allows the user to specify just "this" in OpenACC clauses,
which presumably does the same thing as specifying this[0:1]. For PGI,
but not for OpenACC <= 2.5, that seems to follow a general case for
pointers to structs (2.3. C Structs in OpenACC), "A pointer to a scalar
struct is treated as a one-element array, and should be shaped as
r[0:1]". That's notably different from OpenMP 4.5, which treats plain
mapped pointers as zero-length array sections, and also differs from
the current behaviour of GCC (which bizarrely, IIUC, is to copy the
bits of the host pointer verbatim to the target). OpenACC 2.5 arguably
leaves the behaviour unspecified for pointers without an explicit array
section.

The attached patch allows basic class-wrapping-an-array kinds of
usages, anyway. Re-tested with offloading to nvptx and bootstrapped. OK
to apply?

Thanks,

Julian

2018-09-03  Joseph Myers  <joseph@codesourcery.com>
            Julian Brown  <julian@codesourcery.com>

        PR C++/66053

        * semantics.c (handle_omp_array_sections_1): Allow array
        sections with "this" pointer for OpenACC.

[-- Attachment #2: this-in-openacc-directives-3.diff --]
[-- Type: text/x-patch, Size: 1636 bytes --]

commit 355411e5415f65e09a06f42d400761fff065f7c7
Author: Julian Brown <julian@codesourcery.com>
Date:   Fri Aug 31 17:30:20 2018 -0700

    Allow this[:] array slices for OpenACC
    
    2018-09-03  Joseph Myers  <joseph@codesourcery.com>
    	    Julian Brown  <julian@codesourcery.com>
    
    	* semantics.c (handle_omp_array_sections_1): Allow array sections with
    	"this" pointer for OpenACC.

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 676de01..98511ed 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -4598,7 +4598,8 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree> &types,
 		      omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
 	  return error_mark_node;
 	}
-      else if (TREE_CODE (t) == PARM_DECL
+      else if (ort == C_ORT_OMP
+	       && TREE_CODE (t) == PARM_DECL
 	       && DECL_ARTIFICIAL (t)
 	       && DECL_NAME (t) == this_identifier)
 	{
diff --git a/libgomp/testsuite/libgomp.oacc-c++/this.C b/libgomp/testsuite/libgomp.oacc-c++/this.C
new file mode 100644
index 0000000..510c690
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-c++/this.C
@@ -0,0 +1,43 @@
+#include <cstdlib>
+#include <iostream>
+using namespace std;
+
+class test {
+  public:
+  int a;
+
+  test ()
+  {
+    a = -1;
+#pragma acc enter data copyin (this[0:1])
+  }
+
+  ~test ()
+  {
+#pragma acc exit data delete (this[0:1])
+  }
+
+  void set (int i)
+  {
+    a = i;
+#pragma acc update device (this[0:1])
+  }
+
+  int get ()
+  {
+#pragma acc update host (this[0:1])
+    return a;
+  }
+};
+
+int
+main ()
+{
+  test t;
+
+  t.set (4);
+  if (t.get () != 4)
+    abort ();
+
+  return 0;
+}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053)
  2018-09-03 23:50       ` Julian Brown
@ 2018-10-25 20:08         ` Jason Merrill
  0 siblings, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2018-10-25 20:08 UTC (permalink / raw)
  To: Julian Brown, Jakub Jelinek
  Cc: Nathan Sidwell, gcc-patches List, Joseph S. Myers

On 9/3/18 7:50 PM, Julian Brown wrote:
> On Fri, 31 Aug 2018 16:20:08 +0200
> Jakub Jelinek <jakub@redhat.com> wrote:
> 
>> On Fri, Aug 31, 2018 at 10:04:07AM -0400, Nathan Sidwell wrote:
>>> On 08/30/2018 04:27 PM, Jason Merrill wrote:
>>>    
>>>> On Thu, Aug 30, 2018 at 3:31 PM, Julian Brown
>>>> <julian@codesourcery.com> wrote:
>>>>> "Apart from parsing, it's necessary to prevent the "cannot take
>>>>> the address of 'this', which is an rvalue expression" error
>>>>> from appearing
>>>
>>> Breaking a rather fundamental language attribute does not seem wise.
>>>    
>>>> Why does referring to this[0:1] require making 'this' addressable?
>>>> Surely what we're interested in is the value of 'this', not the
>>>> address.
>>> Yes, transferring the this pointer is very unlikely to be what the
>>> user wants -- the object being referred to contains the data.  It
>>> might be wise to look at the DR's and changes relating to lambdas
>>> and this capture.  Those changes now make it much harder to simply
>>> capture the pointer unintentionally.
>>
>> Yeah, I agree we shouldn't try to make this addressable.
>> Does OpenACC try to map the base of the array section (rather than
>> what e.g. OpenMP does, privatize the pointer base instead and assign
>> the pointer the new value inside of the region)?
>> Even if it is mapped, can't it be mapped by taking an address of a
>> temporary initialized from this?
> 
> For OpenACC, two mappings are created for an array section: one for the
> data (to, from, tofrom, etc.) and a firstprivate pointer with a bias to
> locate the (possibly virtual) zero'th element of the array. I think
> that's the same as OpenMP.
> 
> For the test case given, it's sufficient to merely allow "this" to be
> used as the base pointer for an array section. That usage doesn't
> require "this" to be made addressable.
> 
> The this[0:1] syntax is accepted by PGI
> (https://www.pgroup.com/resources/docs/18.4/x86/openacc-gs/index.htm,
> 2.4 C++ Classes in OpenACC) -- in order to copy "the class itself" to
> the accelerator.
> 
> Referring to class member variables in OpenACC clauses (as the example
> in that section does also) is still problematic in GCC, though.
> 
> PGI also allows the user to specify just "this" in OpenACC clauses,
> which presumably does the same thing as specifying this[0:1]. For PGI,
> but not for OpenACC <= 2.5, that seems to follow a general case for
> pointers to structs (2.3. C Structs in OpenACC), "A pointer to a scalar
> struct is treated as a one-element array, and should be shaped as
> r[0:1]". That's notably different from OpenMP 4.5, which treats plain
> mapped pointers as zero-length array sections, and also differs from
> the current behaviour of GCC (which bizarrely, IIUC, is to copy the
> bits of the host pointer verbatim to the target). OpenACC 2.5 arguably
> leaves the behaviour unspecified for pointers without an explicit array
> section.
> 
> The attached patch allows basic class-wrapping-an-array kinds of
> usages, anyway. Re-tested with offloading to nvptx and bootstrapped. OK
> to apply?
> 
> Thanks,
> 
> Julian
> 
> 2018-09-03  Joseph Myers  <joseph@codesourcery.com>
>              Julian Brown  <julian@codesourcery.com>
> 
>          PR C++/66053
> 
>          * semantics.c (handle_omp_array_sections_1): Allow array
>          sections with "this" pointer for OpenACC.
> 

OK.

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-10-25 19:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-30 19:32 [PATCH, OpenACC] Support C++ "this" in OpenACC directives (PR66053) Julian Brown
2018-08-30 20:27 ` Jason Merrill
2018-08-31 14:04   ` Nathan Sidwell
2018-08-31 14:20     ` Jakub Jelinek
2018-09-03 23:50       ` Julian Brown
2018-10-25 20:08         ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).