diff mbox series

[libgomp,build] Skip plugin-{gcn,hsa} for (-m)x32 (PR bootstrap/93409)

Message ID 2bf0822d-0113-52f9-c81d-8f6692c61bff@codesourcery.com
State New
Headers show
Series [libgomp,build] Skip plugin-{gcn,hsa} for (-m)x32 (PR bootstrap/93409) | expand

Commit Message

Tobias Burnus Jan. 24, 2020, 2:59 p.m. UTC
As reported in PR93409, the build of libgomp/plugin/plugin-gcn.c fails 
with a bunch of error messages when building with 
--with-multilib-list=m32,m64,mx32

The reason is that the GCN plugin assumes 64bit pointers. As with HSA, 
the build is only enabled for x86-64 and "-m32" is excluded. — However, 
it seems as if it makes sense to exclude also "-mx32".

This patch was tested with -m32/-m64 multilib as I do not have a -mx32 
setup.
OK for the trunk?

Tobias

PS: PowerPC support for ROCm was recently added, cf. 
https://www.phoronix.com/scan.php?page=news_item&px=AMDKFD-Compute-POWERPC 
— And, if I understood it correctly, 32bit support would be possible but 
requires work on multiple ends and no one works on it.

Comments

Andrew Stubbs Jan. 27, 2020, 12:20 p.m. UTC | #1
On 24/01/2020 14:59, Tobias Burnus wrote:
> As reported in PR93409, the build of libgomp/plugin/plugin-gcn.c fails 
> with a bunch of error messages when building with 
> --with-multilib-list=m32,m64,mx32
> 
> The reason is that the GCN plugin assumes 64bit pointers. As with HSA, 
> the build is only enabled for x86-64 and "-m32" is excluded. — However, 
> it seems as if it makes sense to exclude also "-mx32".
> 
> This patch was tested with -m32/-m64 multilib as I do not have a -mx32 
> setup.
> OK for the trunk?

This is fine with me, .... but that's probably not enough for this file.

Andrew
Jakub Jelinek Jan. 30, 2020, 9:20 a.m. UTC | #2
On Fri, Jan 24, 2020 at 03:59:28PM +0100, Tobias Burnus wrote:
> As reported in PR93409, the build of libgomp/plugin/plugin-gcn.c fails with
> a bunch of error messages when building with
> --with-multilib-list=m32,m64,mx32
> 
> The reason is that the GCN plugin assumes 64bit pointers. As with HSA, the
> build is only enabled for x86-64 and "-m32" is excluded. — However, it seems
> as if it makes sense to exclude also "-mx32".
> 
> This patch was tested with -m32/-m64 multilib as I do not have a -mx32
> setup.

I don't have any working -mx32 setup around nor any supported GCN offloading
hw around, so can't test anything, thus just a general comment.

In the way LLVM implements offloading, two (or more) separate compilations
starting with preprocessing etc., it is essential to have exactly the same
structure layout in both at least for things, through which the host and
offloading code are interfacing, so one needs say support for offloading
target XYZ do structure layout of ABC host ABI.

The way we implement it in GCC is different, for the structure layout
we perform them pre-IPA in the host compilation, and for offloading
therefore it only matters that the host and offloading target agree on
basics (fundamental types having the same size), the exact structure layout
details shouldn't matter that much.  I don't see the gcn target having
ADJUST_FIELD_ALIGN and all the quirks of the ia32 ABI e.g. with alignment of
long long or double in the structures anyway, yet it seems to be supported
for -m32 (ia32) code, right?  So, I don't see a fundamental reason why
-mx32, which is an ilp32 target like -m32, shouldn't be supported.

By "that much", I mean that while the host vs. offloading target interfaces
should be ok due to structure layout done in the host compiler and then
streamed to the offloading compiler, there is the problem when the
offloaded code interfaces using structures with code natively compiled for
the offloading target (newlib), so say calling stat or similar functions
wouldn't work well.  I'm afraid it won't work well in either offloading
model though, in the GCC way struct stat in the offloaded code will be
simply the host struct stat with field from there and corresponding
structure layout, while in native offloading code probably both different
fields and different structure layout, while in the LLVM model I'd assume
the offloading code will use the offloading target struct stat, most likely
with structure layout from the host.

But looking at the patch, we already disable the plugin for ia32 (-m32), so
I'm fine with your patch.  If it is ever enabled for ia32, it should be
enabled for -mx32 too.

Thus ok for trunk.

	Jakub
Andrew Stubbs Jan. 30, 2020, 9:47 a.m. UTC | #3
On 30/01/2020 09:20, Jakub Jelinek wrote:
> On Fri, Jan 24, 2020 at 03:59:28PM +0100, Tobias Burnus wrote:
>> As reported in PR93409, the build of libgomp/plugin/plugin-gcn.c fails with
>> a bunch of error messages when building with
>> --with-multilib-list=m32,m64,mx32
>>
>> The reason is that the GCN plugin assumes 64bit pointers. As with HSA, the
>> build is only enabled for x86-64 and "-m32" is excluded. — However, it seems
>> as if it makes sense to exclude also "-mx32".
>>
>> This patch was tested with -m32/-m64 multilib as I do not have a -mx32
>> setup.
> 
> I don't have any working -mx32 setup around nor any supported GCN offloading
> hw around, so can't test anything, thus just a general comment.
> 
> In the way LLVM implements offloading, two (or more) separate compilations
> starting with preprocessing etc., it is essential to have exactly the same
> structure layout in both at least for things, through which the host and
> offloading code are interfacing, so one needs say support for offloading
> target XYZ do structure layout of ABC host ABI.
> 
> The way we implement it in GCC is different, for the structure layout
> we perform them pre-IPA in the host compilation, and for offloading
> therefore it only matters that the host and offloading target agree on
> basics (fundamental types having the same size), the exact structure layout
> details shouldn't matter that much.  I don't see the gcn target having
> ADJUST_FIELD_ALIGN and all the quirks of the ia32 ABI e.g. with alignment of
> long long or double in the structures anyway, yet it seems to be supported
> for -m32 (ia32) code, right?  So, I don't see a fundamental reason why
> -mx32, which is an ilp32 target like -m32, shouldn't be supported.

-m32 is not supported. Not in the business sense, anyway. It's never 
been tested, no effort has been made to make it work, and "long int" is 
always 64-bit (as are pointers).

> By "that much", I mean that while the host vs. offloading target interfaces
> should be ok due to structure layout done in the host compiler and then
> streamed to the offloading compiler, there is the problem when the
> offloaded code interfaces using structures with code natively compiled for
> the offloading target (newlib), so say calling stat or similar functions
> wouldn't work well.  I'm afraid it won't work well in either offloading
> model though, in the GCC way struct stat in the offloaded code will be
> simply the host struct stat with field from there and corresponding
> structure layout, while in native offloading code probably both different
> fields and different structure layout, while in the LLVM model I'd assume
> the offloading code will use the offloading target struct stat, most likely
> with structure layout from the host.

Stat is not implemented, nor is any other OS call apart from "write", 
and that's only valid for stdout and stderr (aliased to stdout). It's 
not that semi-hosting could not be implemented; it just hasn't been and 
there's no real use case for it (besides making the testsuite happier). 
If it were to be implemented, however, then indeed x86_64 would be 
significantly simpler than ia32, in many cases.

> But looking at the patch, we already disable the plugin for ia32 (-m32), so
> I'm fine with your patch.  If it is ever enabled for ia32, it should be
> enabled for -mx32 too.
> 
> Thus ok for trunk.
> 
> 	Jakub
>
Jakub Jelinek Jan. 30, 2020, 9:54 a.m. UTC | #4
On Thu, Jan 30, 2020 at 09:47:22AM +0000, Andrew Stubbs wrote:
> > By "that much", I mean that while the host vs. offloading target interfaces
> > should be ok due to structure layout done in the host compiler and then
> > streamed to the offloading compiler, there is the problem when the
> > offloaded code interfaces using structures with code natively compiled for
> > the offloading target (newlib), so say calling stat or similar functions
> > wouldn't work well.  I'm afraid it won't work well in either offloading
> > model though, in the GCC way struct stat in the offloaded code will be
> > simply the host struct stat with field from there and corresponding
> > structure layout, while in native offloading code probably both different
> > fields and different structure layout, while in the LLVM model I'd assume
> > the offloading code will use the offloading target struct stat, most likely
> > with structure layout from the host.
> 
> Stat is not implemented, nor is any other OS call apart from "write", and
> that's only valid for stdout and stderr (aliased to stdout). It's not that
> semi-hosting could not be implemented; it just hasn't been and there's no
> real use case for it (besides making the testsuite happier). If it were to
> be implemented, however, then indeed x86_64 would be significantly simpler
> than ia32, in many cases.

stat was just a random example that first came to my mind, obviously stat
doesn't make much sense in offloading code, but perhaps other functions
that take address of a structure (or reference to it) and are implemented in
the C library (or say libgfortran or libgomp) could.  In gomp it is e.g.
omp_*_lock, though both omp_lock_t and omp_nest_lock_t just contain a single
char array member and all that matters is the @OMP_LOCK_SIZE@ and
@OMP_NEST_LOCK_SIZE@ matches in between host and offloading target (would be
enough if offloading target has it smaller).

	Jakub
diff mbox series

Patch

	PR bootstrap/93409
	* plugin/configfrag.ac (enable_offload_targets): Skip
	HSA and GCN plugin besides -m32 also for -mx32.
	* configure: Regenerate.

diff --git a/libgomp/configure b/libgomp/configure
index 04a6fd96610..404b677e5f1 100755
--- a/libgomp/configure
+++ b/libgomp/configure
@@ -14991,7 +14991,7 @@  fi
 
 # Plugins for offload execution, configure.ac fragment.  -*- mode: autoconf -*-
 #
-# Copyright (C) 2014-2019 Free Software Foundation, Inc.
+# Copyright (C) 2014-2020 Free Software Foundation, Inc.
 #
 # Contributed by Mentor Embedded.
 #
@@ -15320,7 +15320,7 @@  rm -f core conftest.err conftest.$ac_objext \
 	case "${target}" in
 	  x86_64-*-*)
 	    case " ${CC} ${CFLAGS} " in
-	      *" -m32 "*)
+	      *" -m32 "*|*" -mx32 "*)
 	        PLUGIN_HSA=0
 		;;
 	      *)
@@ -15360,7 +15360,7 @@  rm -f core conftest.err conftest.$ac_objext \
 	case "${target}" in
 	  x86_64-*-*)
 	    case " ${CC} ${CFLAGS} " in
-	      *" -m32 "*)
+	      *" -m32 "*|*" -mx32 "*)
 		PLUGIN_GCN=0
 		;;
 	      *)
diff --git a/libgomp/plugin/configfrag.ac b/libgomp/plugin/configfrag.ac
index 9a424aa1c9c..fc91702a434 100644
--- a/libgomp/plugin/configfrag.ac
+++ b/libgomp/plugin/configfrag.ac
@@ -211,7 +211,7 @@  if test x"$enable_offload_targets" != x; then
 	case "${target}" in
 	  x86_64-*-*)
 	    case " ${CC} ${CFLAGS} " in
-	      *" -m32 "*)
+	      *" -m32 "*|*" -mx32 "*)
 	        PLUGIN_HSA=0
 		;;
 	      *)
@@ -251,7 +251,7 @@  if test x"$enable_offload_targets" != x; then
 	case "${target}" in
 	  x86_64-*-*)
 	    case " ${CC} ${CFLAGS} " in
-	      *" -m32 "*)
+	      *" -m32 "*|*" -mx32 "*)
 		PLUGIN_GCN=0
 		;;
 	      *)