[libgomp,nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support

Message ID 26b4617e-7452-3b67-c29d-2d9f0c39d3e8@suse.de
State New
Headers show
Series
  • [libgomp,nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support
Related show

Commit Message

Tom de Vries Dec. 12, 2018, 8:29 a.m.
[ was: Re: [committed 2/4] (Partial) OpenMP 5.0 support for GCC 9
(runtime library changes) ]

On 08-11-18 18:18, Jakub Jelinek wrote:
> Hi!

> 

> This is the libgomp/ except libgomp/testsuite/ part of the gomp-5_0-branch

> merge to trunk I've just committed.

> 

> 2018-11-08  Jakub Jelinek  <jakub@redhat.com>

> 

> 	* affinity.c (gomp_display_affinity_place): New function.

> 	* affinity-fmt.c: New file.


> 	* fortran.c: Include stdio.h and string.h.

> 	(omp_pause_resource, omp_pause_resource_all): Add ialias_redirect.

> 	(omp_get_schedule_, omp_get_schedule_8_): Mask off GFS_MONOTONIC bit.

> 	(omp_set_affinity_format_, omp_get_affinity_format_,

> 	omp_display_affinity_, omp_capture_affinity_, omp_pause_resource_,

> 	omp_pause_resource_all_): New functions.


Hi,

OK for trunk?

Thanks,
- Tom

Comments

Jakub Jelinek Dec. 12, 2018, 9:36 a.m. | #1
On Wed, Dec 12, 2018 at 09:29:36AM +0100, Tom de Vries wrote:
> > This is the libgomp/ except libgomp/testsuite/ part of the gomp-5_0-branch

> > merge to trunk I've just committed.

> > 

> > 2018-11-08  Jakub Jelinek  <jakub@redhat.com>

> > 

> > 	* affinity.c (gomp_display_affinity_place): New function.

> > 	* affinity-fmt.c: New file.

> 

> > 	* fortran.c: Include stdio.h and string.h.

> > 	(omp_pause_resource, omp_pause_resource_all): Add ialias_redirect.

> > 	(omp_get_schedule_, omp_get_schedule_8_): Mask off GFS_MONOTONIC bit.

> > 	(omp_set_affinity_format_, omp_get_affinity_format_,

> > 	omp_display_affinity_, omp_capture_affinity_, omp_pause_resource_,

> > 	omp_pause_resource_all_): New functions.

> 

> OK for trunk?

> 

> Thanks,

> - Tom


> [libgomp, nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support

> 

> Disable compilation of support for OMP_DISPLAY_AFFINITY and OMP_AFFINITY_FORMAT

> for nvptx.

> 

> This fixes some libgomp testsuite failures for x86_64 with nvptx accelerator.

> 

> Build on x86_64 with nvptx accelerator, tested libgomp.

> 

> 2018-12-12  Tom de Vries  <tdevries@suse.de>

> 

> 	* affinity.c (gomp_display_affinity_place): Guard with

> 	#ifndef LIBGOMP_OFFLOADED_ONLY.

> 	* fortran.c (omp_set_affinity_format_, omp_get_affinity_format_)

> 	(omp_display_affinity_, omp_capture_affinity_): Same.

> 	* libgomp/config/nvptx/affinity-fmt.c: New, empty file overriding

> 	libgomp/affinity-fmt.c.


The runtime APIs should still be available inside of OpenMP regions, your
patch makes them unavailable.
What exactly doesn't work?
hostname handling, getpid, thread_id, affinity places?
I'd prefer for now to just stub what doesn't work in the LIBGOMP_OFFLOADED_ONLY
case (print "node", 0 as pid, 0 as thread id, something for affinity
(0 or 0-N where N is the number of warps it can support or whatever).

Eventually we need to find a way how to transfer some ICVs and other data
from the host to the offloading library, either process shared that aren't
changing, which can include the hostname, getpid, or some others that would
need to be passed for every target region.

	Jakub
Tom de Vries Dec. 12, 2018, 10:48 a.m. | #2
On 12-12-18 10:36, Jakub Jelinek wrote:
> On Wed, Dec 12, 2018 at 09:29:36AM +0100, Tom de Vries wrote:

>>> This is the libgomp/ except libgomp/testsuite/ part of the gomp-5_0-branch

>>> merge to trunk I've just committed.

>>>

>>> 2018-11-08  Jakub Jelinek  <jakub@redhat.com>

>>>

>>> 	* affinity.c (gomp_display_affinity_place): New function.

>>> 	* affinity-fmt.c: New file.

>>

>>> 	* fortran.c: Include stdio.h and string.h.

>>> 	(omp_pause_resource, omp_pause_resource_all): Add ialias_redirect.

>>> 	(omp_get_schedule_, omp_get_schedule_8_): Mask off GFS_MONOTONIC bit.

>>> 	(omp_set_affinity_format_, omp_get_affinity_format_,

>>> 	omp_display_affinity_, omp_capture_affinity_, omp_pause_resource_,

>>> 	omp_pause_resource_all_): New functions.

>>

>> OK for trunk?

>>

>> Thanks,

>> - Tom

> 

>> [libgomp, nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support

>>

>> Disable compilation of support for OMP_DISPLAY_AFFINITY and OMP_AFFINITY_FORMAT

>> for nvptx.

>>

>> This fixes some libgomp testsuite failures for x86_64 with nvptx accelerator.

>>

>> Build on x86_64 with nvptx accelerator, tested libgomp.

>>

>> 2018-12-12  Tom de Vries  <tdevries@suse.de>

>>

>> 	* affinity.c (gomp_display_affinity_place): Guard with

>> 	#ifndef LIBGOMP_OFFLOADED_ONLY.

>> 	* fortran.c (omp_set_affinity_format_, omp_get_affinity_format_)

>> 	(omp_display_affinity_, omp_capture_affinity_): Same.

>> 	* libgomp/config/nvptx/affinity-fmt.c: New, empty file overriding

>> 	libgomp/affinity-fmt.c.

> 

> The runtime APIs should still be available inside of OpenMP regions, your

> patch makes them unavailable.


Ah, I see.

> What exactly doesn't work?

> hostname handling, getpid, thread_id, affinity places?


The concrete error I'm getting is unresolved symbol getpid.  This is
caused by the AC_COMPILE_IFELSE configure test for HAVE_GETPID passing
for nvptx, while the master newlib for nvptx does not contain getpid.

> I'd prefer for now to just stub what doesn't work in the LIBGOMP_OFFLOADED_ONLY

> case (print "node", 0 as pid, 0 as thread id, something for affinity

> (0 or 0-N where N is the number of warps it can support or whatever).

> 


OK, I'll give that a try.

Thanks,
- Tom

> Eventually we need to find a way how to transfer some ICVs and other data

> from the host to the offloading library, either process shared that aren't

> changing, which can include the hostname, getpid, or some others that would

> need to be passed for every target region.
Thomas Schwinge Dec. 12, 2018, 1 p.m. | #3
Hi!

On Wed, 12 Dec 2018 10:36:17 +0100, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 12, 2018 at 09:29:36AM +0100, Tom de Vries wrote:

> > > This is the libgomp/ except libgomp/testsuite/ part of the gomp-5_0-branch

> > > merge to trunk I've just committed.


> > [libgomp, nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support


Thanks, Tom, for looking into these issues.  I'd also noticed these, but
not yet been able to allocate time to resolve them.  I'll test your
patches, too.


> Eventually we need to find a way how to transfer some ICVs and other data

> from the host to the offloading library, either process shared that aren't

> changing, which can include the hostname, getpid, or some others that would

> need to be passed for every target region.


That would probably also include any state that the respective language
runtime libraries maintain?  For example, C's global rounding mode as set
my "fesetround".  ..., and I now wonder how to propagate that from the
host libc to the target libcs, for example host: glibc vs. nvptx
offloading: newlib -- have to do a host-side "fegetround" before each
offloaded code region, or at least when there's been an intermediate
host-side "fesetround" call (so have to track these?), and likewise for
any other such state-changing functions?  Also, the "options" array and
call of "_gfortran_set_options" that the Fortran front end synthesizes
into each "main" function?

I have not yet looked into these things in detail, however.


Grüße
 Thomas
Tom de Vries Dec. 12, 2018, 1:02 p.m. | #4
On 12-12-18 11:48, Tom de Vries wrote:
> On 12-12-18 10:36, Jakub Jelinek wrote:

>> On Wed, Dec 12, 2018 at 09:29:36AM +0100, Tom de Vries wrote:

>>>> This is the libgomp/ except libgomp/testsuite/ part of the gomp-5_0-branch

>>>> merge to trunk I've just committed.

>>>>

>>>> 2018-11-08  Jakub Jelinek  <jakub@redhat.com>

>>>>

>>>> 	* affinity.c (gomp_display_affinity_place): New function.

>>>> 	* affinity-fmt.c: New file.

>>>

>>>> 	* fortran.c: Include stdio.h and string.h.

>>>> 	(omp_pause_resource, omp_pause_resource_all): Add ialias_redirect.

>>>> 	(omp_get_schedule_, omp_get_schedule_8_): Mask off GFS_MONOTONIC bit.

>>>> 	(omp_set_affinity_format_, omp_get_affinity_format_,

>>>> 	omp_display_affinity_, omp_capture_affinity_, omp_pause_resource_,

>>>> 	omp_pause_resource_all_): New functions.

>>>

>>> OK for trunk?

>>>

>>> Thanks,

>>> - Tom

>>

>>> [libgomp, nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support

>>>

>>> Disable compilation of support for OMP_DISPLAY_AFFINITY and OMP_AFFINITY_FORMAT

>>> for nvptx.

>>>

>>> This fixes some libgomp testsuite failures for x86_64 with nvptx accelerator.

>>>

>>> Build on x86_64 with nvptx accelerator, tested libgomp.

>>>

>>> 2018-12-12  Tom de Vries  <tdevries@suse.de>

>>>

>>> 	* affinity.c (gomp_display_affinity_place): Guard with

>>> 	#ifndef LIBGOMP_OFFLOADED_ONLY.

>>> 	* fortran.c (omp_set_affinity_format_, omp_get_affinity_format_)

>>> 	(omp_display_affinity_, omp_capture_affinity_): Same.

>>> 	* libgomp/config/nvptx/affinity-fmt.c: New, empty file overriding

>>> 	libgomp/affinity-fmt.c.

>>

>> The runtime APIs should still be available inside of OpenMP regions, your

>> patch makes them unavailable.

> 

> Ah, I see.

> 

>> What exactly doesn't work?

>> hostname handling, getpid, thread_id, affinity places?

> 

> The concrete error I'm getting is unresolved symbol getpid.  This is

> caused by the AC_COMPILE_IFELSE configure test for HAVE_GETPID passing

> for nvptx, while the master newlib for nvptx does not contain getpid.

> 

>> I'd prefer for now to just stub what doesn't work in the LIBGOMP_OFFLOADED_ONLY

>> case (print "node", 0 as pid, 0 as thread id, something for affinity

>> (0 or 0-N where N is the number of warps it can support or whatever).

>>

> 

> OK, I'll give that a try.

> 


This RFC patch implements that approach for getpid and gethostname (I
wonder though whether it's not possible to turn the corresponding
configure tests into link tests, which could also fix this for nvptx).

Another problem we're running into is missing istty, pulled in by
fwrite. The nvptx newlib port does support write, but I'm unsure in what
form I should handle this.  Add configure tests for fwrite and write? Or
special case the files in the config/nvptx dir? Any advice here?

Thanks,
- Tom
[RFC][libgomp, nvptx] Fix target-5.c compilation

Libgomp test-case libgomp.c/target-5.c is failing to compile with nvptx
accelerator due to missing:
- getpid
- gethostname
- isatty (pulled in by fwrite)
in the nvptx newlib.

This demonstrator patch fixes that by minimally guarding all related locations
with ifndef LIBGOMP_OFFLOADED_ONLY, which allows the test-case (and others) to
pass.

---
 libgomp/affinity-fmt.c | 12 +++++++++++-
 libgomp/fortran.c      |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/libgomp/affinity-fmt.c b/libgomp/affinity-fmt.c
index 9e5c5f754eb..72b511eec73 100644
--- a/libgomp/affinity-fmt.c
+++ b/libgomp/affinity-fmt.c
@@ -184,6 +184,7 @@ static void
 gomp_display_hostname (char *buffer, size_t size, size_t *ret,
 		       bool right, size_t sz)
 {
+#ifndef LIBGOMP_OFFLOADED_ONLY
 #ifdef HAVE_GETHOSTNAME
   {
     char buf[256];
@@ -227,6 +228,7 @@ gomp_display_hostname (char *buffer, size_t size, size_t *ret,
 	return;
       }
   }
+#endif
 #endif
   gomp_display_string_len (buffer, size, ret, right, sz, "node", 4);
 }
@@ -351,7 +353,7 @@ gomp_display_affinity (char *buffer, size_t size,
 	  gomp_display_hostname (buffer, size, &ret, right, sz);
 	  break;
 	case 'P':
-#ifdef HAVE_GETPID
+#if defined(HAVE_GETPID) && !defined(LIBGOMP_OFFLOADED_ONLY)
 	  val = getpid ();
 #else
 	  val = 0;
@@ -456,13 +458,17 @@ omp_display_affinity (const char *format)
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
       fwrite (buf, 1, ret + 1, stderr);
+#endif
       return;
     }
   b = gomp_malloc (ret + 1);
   ialias_call (omp_capture_affinity) (b, ret + 1, format);
   b[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
   fwrite (b, 1, ret + 1, stderr);
+#endif
   free (b);
 }
 
@@ -477,13 +483,17 @@ gomp_display_affinity_thread (gomp_thread_handle handle,
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
       fwrite (buf, 1, ret + 1, stderr);
+#endif
       return;
     }
   b = gomp_malloc (ret + 1);
   gomp_display_affinity (b, ret + 1, gomp_affinity_format_var,
   			 handle, ts, place);
   b[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
   fwrite (b, 1, ret + 1, stderr);
+#endif
   free (b);
 }
diff --git a/libgomp/fortran.c b/libgomp/fortran.c
index 0157baec648..21ea9482e09 100644
--- a/libgomp/fortran.c
+++ b/libgomp/fortran.c
@@ -626,7 +626,9 @@ omp_display_affinity_ (const char *format, size_t format_len)
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
       fwrite (buf, 1, ret + 1, stderr);
+#endif
     }
   else
     {
@@ -635,7 +637,9 @@ omp_display_affinity_ (const char *format, size_t format_len)
 			     format_len ? fmt : gomp_affinity_format_var,
 			     gomp_thread_self (), &thr->ts, thr->place);
       b[ret] = '\n';
+#ifndef LIBGOMP_OFFLOADED_ONLY
       fwrite (b, 1, ret + 1, stderr);
+#endif
       free (b);
     }
   if (fmt && fmt != fmt_buf)
Jakub Jelinek Dec. 12, 2018, 1:15 p.m. | #5
On Wed, Dec 12, 2018 at 02:02:20PM +0100, Tom de Vries wrote:
> This RFC patch implements that approach for getpid and gethostname (I

> wonder though whether it's not possible to turn the corresponding

> configure tests into link tests, which could also fix this for nvptx).

> 

> Another problem we're running into is missing istty, pulled in by

> fwrite. The nvptx newlib port does support write, but I'm unsure in what

> form I should handle this.  Add configure tests for fwrite and write? Or

> special case the files in the config/nvptx dir? Any advice here?

> 

> Thanks,

> - Tom


> [RFC][libgomp, nvptx] Fix target-5.c compilation

> 

> Libgomp test-case libgomp.c/target-5.c is failing to compile with nvptx

> accelerator due to missing:

> - getpid

> - gethostname

> - isatty (pulled in by fwrite)

> in the nvptx newlib.

> 

> This demonstrator patch fixes that by minimally guarding all related locations

> with ifndef LIBGOMP_OFFLOADED_ONLY, which allows the test-case (and others) to

> pass.


I guess my preference would be something similar to what
libgomp/config/mingw32/affinity-fmt.c does now, so override the file,
define/undefine something in there and include the common affinity-fmt.c.
Perhaps for the fwrite stuff define introduce a function that does that
(say
void
gomp_print_string (const char *str, size_t len)
{
  fwrite (str, 1, len, stderr);
},
because it is in more than one file, make it hidden in libgomp.h.
config/nvptx/error.c then has what you could also use in
config/nvptx/affinity-fmt.c.
And for hostname/getpid maybe just #undef those with a comment?

	Jakub

Patch

[libgomp, nvptx] Disable OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support

Disable compilation of support for OMP_DISPLAY_AFFINITY and OMP_AFFINITY_FORMAT
for nvptx.

This fixes some libgomp testsuite failures for x86_64 with nvptx accelerator.

Build on x86_64 with nvptx accelerator, tested libgomp.

2018-12-12  Tom de Vries  <tdevries@suse.de>

	* affinity.c (gomp_display_affinity_place): Guard with
	#ifndef LIBGOMP_OFFLOADED_ONLY.
	* fortran.c (omp_set_affinity_format_, omp_get_affinity_format_)
	(omp_display_affinity_, omp_capture_affinity_): Same.
	* libgomp/config/nvptx/affinity-fmt.c: New, empty file overriding
	libgomp/affinity-fmt.c.

---
 libgomp/affinity.c                  | 2 ++
 libgomp/config/nvptx/affinity-fmt.c | 0
 libgomp/fortran.c                   | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/libgomp/affinity.c b/libgomp/affinity.c
index efe5c4260e3..dbfb8e7a75d 100644
--- a/libgomp/affinity.c
+++ b/libgomp/affinity.c
@@ -140,6 +140,7 @@  gomp_get_place_proc_ids_8 (int place_num, int64_t *ids)
   (void) ids;
 }
 
+#ifndef LIBGOMP_OFFLOADED_ONLY
 void
 gomp_display_affinity_place (char *buffer, size_t size, size_t *ret,
 			     int place)
@@ -151,6 +152,7 @@  gomp_display_affinity_place (char *buffer, size_t size, size_t *ret,
     strcpy (buf, "0");
   gomp_display_string (buffer, size, ret, buf, strlen (buf));
 }
+#endif
 
 ialias(omp_get_place_num_procs)
 ialias(omp_get_place_proc_ids)
diff --git a/libgomp/config/nvptx/affinity-fmt.c b/libgomp/config/nvptx/affinity-fmt.c
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/libgomp/fortran.c b/libgomp/fortran.c
index 0157baec648..a577a2e515b 100644
--- a/libgomp/fortran.c
+++ b/libgomp/fortran.c
@@ -584,6 +584,7 @@  omp_get_max_task_priority_ (void)
   return omp_get_max_task_priority ();
 }
 
+#ifndef LIBGOMP_OFFLOADED_ONLY
 void
 omp_set_affinity_format_ (const char *format, size_t format_len)
 {
@@ -664,6 +665,7 @@  omp_capture_affinity_ (char *buffer, const char *format,
     memset (buffer + ret, ' ', buffer_len - ret);
   return ret;
 }
+#endif
 
 int32_t
 omp_pause_resource_ (const int32_t *kind, const int32_t *device_num)