diff mbox

[openacc] on_device fix

Message ID 5632B856.4050509@acm.org
State New
Headers show

Commit Message

Nathan Sidwell Oct. 30, 2015, 12:22 a.m. UTC 
acc_on_device and it's builtin had a conflict.  The function formally takes an 
enum argument, but the builtin takes an int -- primarily to avoid the compiler 
having to generate the enum  type internally.

This works fine for C,  where the external declaration of the function (in 
openacc.h) matches up with the builtin, and we optimize the builtin as expected.

It fails for C++ where the builtin doesn't match the declaration in the header. 
  We end up with emitting a call to acc_on_device,  which is resolved by 
libgomp.  Unfortunately that means we fail to optimize.

We could resolve this in a number of ways

1) make the header file's declaration have an int argument.

2) make the header file have an inline definition  fowarding to a differently 
named function with an int argument, that matched a renamed builtin

3) Do what this patch does.

Option 1 would be visible in the type system, if someone took the address of the 
function (I'm not sure why they'd do that).  We used this variant on the gomp4 
branch for a long time.

Option  2 requires changing the symbols exported from libgomp.   Instead of 
exporting acc_on_device we'd need to export __acc_on_device or something.  And 
we'd need to provide a backwards compatible entry point named acc_on_device anyway.

Option 3 leaves things unchanged for C --  declare a function with an enum arg. 
  But for C++ we the extern "C" declaration takes an int -- and therefore 
matches the builtin.  We insert an inline wrapper that takes an enum argument. 
Because of C++'s overload resolution both the wrapper and the int-taking 
declaration can have the same source name.

We require the enum to be layout compatible  with int -- this was an artifact of 
the earlier implementation anyway.  I added enumeration values to acc_device_t 
to enforce that, just in case someone tries to compile their openacc code with 
-fshort-enums.

Committed to trunk.

nathan
diff mbox

Patch

2015-10-29  Nathan Sidwell  <nathan@codesourcery.com>

	gcc/
	* openacc.h (enum acc_device_t): Reformat. Ensure layout
	compatibility.
	(enum acc_async_t): Reformat.
	(acc_on_device): Declare compatible with builtin and provide C++
	wrapper.
	* testsuite/libgomp.oacc-c-c++-common/acc-on-device.c: New.

	gcc/testsuite/
	* c-c++-common/goacc/acc_on_device-2-off.c: Delete.
	* c-c++-common/goacc/acc_on_device-2.c: Delete.

Index: libgomp/openacc.h
===================================================================
--- libgomp/openacc.h	(revision 229535)
+++ libgomp/openacc.h	(working copy)
@@ -47,24 +47,25 @@  extern "C" {
 #endif
 
 /* Types */
-typedef enum acc_device_t
-  {
-    /* Keep in sync with include/gomp-constants.h.  */
-    acc_device_none = 0,
-    acc_device_default = 1,
-    acc_device_host = 2,
-    /* acc_device_host_nonshm = 3 removed.  */
-    acc_device_not_host = 4,
-    acc_device_nvidia = 5,
-    _ACC_device_hwm
-  } acc_device_t;
-
-typedef enum acc_async_t
-  {
-    /* Keep in sync with include/gomp-constants.h.  */
-    acc_async_noval = -1,
-    acc_async_sync  = -2
-  } acc_async_t;
+typedef enum acc_device_t {
+  /* Keep in sync with include/gomp-constants.h.  */
+  acc_device_none = 0,
+  acc_device_default = 1,
+  acc_device_host = 2,
+  /* acc_device_host_nonshm = 3 removed.  */
+  acc_device_not_host = 4,
+  acc_device_nvidia = 5,
+  _ACC_device_hwm,
+  /* Ensure enumeration is layout compatible with int.  */
+  _ACC_highest = __INT_MAX__,
+  _ACC_neg = -1
+} acc_device_t;
+
+typedef enum acc_async_t {
+  /* Keep in sync with include/gomp-constants.h.  */
+  acc_async_noval = -1,
+  acc_async_sync  = -2
+} acc_async_t;
 
 int acc_get_num_devices (acc_device_t) __GOACC_NOTHROW;
 void acc_set_device_type (acc_device_t) __GOACC_NOTHROW;
@@ -79,7 +80,11 @@  void acc_wait_all (void) __GOACC_NOTHROW
 void acc_wait_all_async (int) __GOACC_NOTHROW;
 void acc_init (acc_device_t) __GOACC_NOTHROW;
 void acc_shutdown (acc_device_t) __GOACC_NOTHROW;
-int acc_on_device (acc_device_t) __GOACC_NOTHROW;
+#ifdef __cplusplus
+int acc_on_device (int __arg) __GOACC_NOTHROW;
+#else
+int acc_on_device (acc_device_t __arg) __GOACC_NOTHROW;
+#endif
 void *acc_malloc (size_t) __GOACC_NOTHROW;
 void acc_free (void *) __GOACC_NOTHROW;
 /* Some of these would be more correct with const qualifiers, but
@@ -113,6 +118,13 @@  int acc_set_cuda_stream (int, void *) __
 
 #ifdef __cplusplus
 }
+
+/* Forwarding function with correctly typed arg.  */
+
+inline int acc_on_device (acc_device_t __arg) __GOACC_NOTHROW
+{
+  return acc_on_device ((int) __arg);
+}
 #endif
 
 #endif /* _OPENACC_H */
Index: libgomp/testsuite/libgomp.oacc-c-c++-common/acc-on-device.c
===================================================================
--- libgomp/testsuite/libgomp.oacc-c-c++-common/acc-on-device.c	(revision 0)
+++ libgomp/testsuite/libgomp.oacc-c-c++-common/acc-on-device.c	(working copy)
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-O2" } */
+
+#include <openacc.h>
+
+int Foo (acc_device_t x)
+{
+  return acc_on_device (x);
+}
+
+/* { dg-final { scan-assembler-not "acc_on_device" } } */
Index: gcc/testsuite/c-c++-common/goacc/acc_on_device-2-off.c
===================================================================
--- gcc/testsuite/c-c++-common/goacc/acc_on_device-2-off.c	(revision 229535)
+++ gcc/testsuite/c-c++-common/goacc/acc_on_device-2-off.c	(working copy)
@@ -1,24 +0,0 @@ 
-/* Have to enable optimizations, as otherwise builtins won't be expanded.  */
-/* { dg-additional-options "-O -fdump-rtl-expand -fno-openacc" } */
-
-#if __cplusplus
-extern "C" {
-#endif
-
-typedef enum acc_device_t { acc_device_X = 123 } acc_device_t;
-extern int acc_on_device (acc_device_t);
-
-#if __cplusplus
-}
-#endif
-
-int
-f (void)
-{
-  const acc_device_t dev = acc_device_X;
-  return acc_on_device (dev);
-}
-
-/* Without -fopenacc, we're expecting one call.
-   { dg-final { scan-rtl-dump-times "\\\(call \[^\\n\]* acc_on_device" 1 "expand" } } */
-
Index: gcc/testsuite/c-c++-common/goacc/acc_on_device-2.c
===================================================================
--- gcc/testsuite/c-c++-common/goacc/acc_on_device-2.c	(revision 229535)
+++ gcc/testsuite/c-c++-common/goacc/acc_on_device-2.c	(working copy)
@@ -1,28 +0,0 @@ 
-/* Have to enable optimizations, as otherwise builtins won't be expanded.  */
-/* { dg-additional-options "-O -fdump-rtl-expand" } */
-
-#if __cplusplus
-extern "C" {
-#endif
-
-typedef enum acc_device_t { acc_device_X = 123 } acc_device_t;
-extern int acc_on_device (acc_device_t);
-
-#if __cplusplus
-}
-#endif
-
-int
-f (void)
-{
-  const acc_device_t dev = acc_device_X;
-  return acc_on_device (dev);
-}
-
-/* With -fopenacc, we're expecting the builtin to be expanded, so no calls.
-   TODO: in C++, even under extern "C", the use of enum for acc_device_t
-   perturbs expansion as a builtin, which expects an int parameter.  It's fine
-   when changing acc_device_t to plain int, but that's not what we're doing in
-   <openacc.h>.
-
-   { dg-final { scan-rtl-dump-times "\\\(call \[^\\n\]* acc_on_device" 0 "expand" { xfail c++ } } } */