Re: [patch] [gcn][nvptx] Add warning to mkoffload for 32bit host code
Hi!
On 2024-04-25T16:07:53+0100, Andrew Stubbs wrote:
> On 25/04/2024 11:51, Tobias Burnus wrote:
>> Motivated by a surprise of a colleague that with -m32,
>> no offload dumps were created; that's because mkoffload
>> does not process host binaries when the are 32bit (i.e. ilp32).
>>
>> Internally, that done as follows: The host compiler passes to
>> 'mkoffload' the used host ABI, i.e. -foffload-abi=ilp32 or -foffload-abi=lp64
>>
>> That's done via TARGET_OFFLOAD_OPTIONS, which is supported by aarch64, i386,
>> and rs6000.
>>
>> While it is sensible (albeit not strictly required) that GCC requires that
>> the host and device side agree and that only 64bit is implemented for the
>> device side, it can be confusing that silently no offloading code is
>> generated.
>>
>>
>> Hence, I propose to print a warning in that case - as implemented in the
>> attached patch:
>>
>> $ gcc -fopenmp -m32 test.c
>> nvptx mkoffload: warning: offload code generation skipped: offloading with
>> 32-bit host code is currently not supported
>> gcn mkoffload: warning: offload code generation skipped: offloading with
>> 32-bit host code is currently not supported
>>
>> * * *
>>
>> This shouldn't have any effect on offload builds using -m64
>> and non-offload builds – while several testcases already have
>> issues with '-m32' when offloading is enabled or an offloading
>> device is available.
>>
>> To make it not worse, this patch adds some pruning and for
>> a subset of the failing testcases, I added code to avoids FAILS.
>> There are some more fails, but those aren't new.
>>
>> Comments, remarks, suggestions?
For "continuity", please reference "PR libgomp/65099" in the Git commit
log.
>> Is the mkoffload.cc part is okay?
>
> The mkoffload part looks reasonable to me. I'm not sure if there are
> other ABIs we might want to warn about
Right, let's please generalize this -- warn for all cases that we don't
support. That is, instead of:
if (offload_abi == OFFLOAD_ABI_ILP32)
[warning]
else if (offload_abi == OFFLOAD_ABI_LP64)
{
[...]
..., use:
if (offload_abi != OFFLOAD_ABI_LP64)
[warning]
else
{
[...]
For the 'warning' diagnostic, you may use 'const char *abi' (as currently
present in the GCN 'mkoffload'; similarly adapt into the nvptx one).
Maybe:
warning (0, "offload code generation skipped: currently not supported for
%qs", abi);
Similarly then adjust 'libgomp-dg-prune', and in the test cases, use
'target lp64' (untested) instead of 'target { ! ia32 }', to match what
we're doing in the 'mkoffload's, and also correspondingly adjust the
"Skip for ia32 [...]" comments.
Instead of:
- { dg-note {requires-7-aux\.c' has 'unified_address'} {} { xfail *-*-*
} 0 }
+ { dg-note {requires-7-aux\.c' has 'unified_address'} {} { xfail {
*-*-* } } 0 }
..., I think you're looking for (untested):
- { dg-note {requires-7-aux\.c' has 'unified_address'} {} { xfail *-*-*
} 0 }
+ { dg-note {requires-7-aux\.c' has 'unified_address'} {} { target lp64
xfail *-*-* } 0 }
(I've not generally reviewed necessary test suite changes.)
> but this is definitely an
> improvement.
Right, thanks!
A follow-up change could then make sure that 'offload_target_amdgcn' etc.
only return true if offloading compilation actually is supported given
the current command-line options. Either again via a
'check_effective_target_lp64' check in
'libgomp_check_effective_target_offload_target', or -- better? -- change
the driver to only conditionally print 'OFFLOAD_TARGET_NAMES=[...]'?
Does that basically mean to move the 'offload_abi' checks from the
'mkoffload's into the driver? That appears to make sense indeed.
(..., so that we don't even invoke the 'mkoffload's for unsupported
configurations/options.)
Grüße
Thomas
[nvptx] *ping* - [patch] [gcn][nvptx] Add warning to mkoffload for 32bit host code
Hi Thomas, hi Tom, any comment regarding this patch? https://gcc.gnu.org/pipermail/gcc-patches/2024-April/650007.html Tobias Am 25.04.24 um 12:51 schrieb Tobias Burnus: Motivated by a surprise of a colleague that with -m32, no offload dumps were created; that's because mkoffload does not process host binaries when the are 32bit (i.e. ilp32). Internally, that done as follows: The host compiler passes to 'mkoffload' the used host ABI, i.e. -foffload-abi=ilp32 or -foffload-abi=lp64 That's done via TARGET_OFFLOAD_OPTIONS, which is supported by aarch64, i386, and rs6000. While it is sensible (albeit not strictly required) that GCC requires that the host and device side agree and that only 64bit is implemented for the device side, it can be confusing that silently no offloading code is generated. Hence, I propose to print a warning in that case - as implemented in the attached patch: $ gcc -fopenmp -m32 test.c nvptx mkoffload: warning: offload code generation skipped: offloading with 32-bit host code is currently not supported gcn mkoffload: warning: offload code generation skipped: offloading with 32-bit host code is currently not supported * * * This shouldn't have any effect on offload builds using -m64 and non-offload builds – while several testcases already have issues with '-m32' when offloading is enabled or an offloading device is available. To make it not worse, this patch adds some pruning and for a subset of the failing testcases, I added code to avoids FAILS. There are some more fails, but those aren't new. Comments, remarks, suggestions? Is the mkoffload.cc part is okay? Tobias
Re: [patch] [gcn][nvptx] Add warning to mkoffload for 32bit host code
On 25/04/2024 11:51, Tobias Burnus wrote: Motivated by a surprise of a colleague that with -m32, no offload dumps were created; that's because mkoffload does not process host binaries when the are 32bit (i.e. ilp32). Internally, that done as follows: The host compiler passes to 'mkoffload' the used host ABI, i.e. -foffload-abi=ilp32 or -foffload-abi=lp64 That's done via TARGET_OFFLOAD_OPTIONS, which is supported by aarch64, i386, and rs6000. While it is sensible (albeit not strictly required) that GCC requires that the host and device side agree and that only 64bit is implemented for the device side, it can be confusing that silently no offloading code is generated. Hence, I propose to print a warning in that case - as implemented in the attached patch: $ gcc -fopenmp -m32 test.c nvptx mkoffload: warning: offload code generation skipped: offloading with 32-bit host code is currently not supported gcn mkoffload: warning: offload code generation skipped: offloading with 32-bit host code is currently not supported * * * This shouldn't have any effect on offload builds using -m64 and non-offload builds – while several testcases already have issues with '-m32' when offloading is enabled or an offloading device is available. To make it not worse, this patch adds some pruning and for a subset of the failing testcases, I added code to avoids FAILS. There are some more fails, but those aren't new. Comments, remarks, suggestions? Is the mkoffload.cc part is okay? The mkoffload part looks reasonable to me. I'm not sure if there are other ABIs we might want to warn about, but this is definitely an improvement. Andrew
[patch] [gcn][nvptx] Add warning to mkoffload for 32bit host code
Motivated by a surprise of a colleague that with -m32,
no offload dumps were created; that's because mkoffload
does not process host binaries when the are 32bit (i.e. ilp32).
Internally, that done as follows: The host compiler passes to
'mkoffload' the used host ABI, i.e. -foffload-abi=ilp32 or -foffload-abi=lp64
That's done via TARGET_OFFLOAD_OPTIONS, which is supported by aarch64, i386,
and rs6000.
While it is sensible (albeit not strictly required) that GCC requires that
the host and device side agree and that only 64bit is implemented for the
device side, it can be confusing that silently no offloading code is generated.
Hence, I propose to print a warning in that case - as implemented in the
attached patch:
$ gcc -fopenmp -m32 test.c
nvptx mkoffload: warning: offload code generation skipped: offloading with
32-bit host code is currently not supported
gcn mkoffload: warning: offload code generation skipped: offloading with 32-bit
host code is currently not supported
* * *
This shouldn't have any effect on offload builds using -m64
and non-offload builds – while several testcases already have
issues with '-m32' when offloading is enabled or an offloading
device is available.
To make it not worse, this patch adds some pruning and for
a subset of the failing testcases, I added code to avoids FAILS.
There are some more fails, but those aren't new.
Comments, remarks, suggestions?
Is the mkoffload.cc part is okay?
Tobias
[gcn][nvptx] Add warning to mkoffload for 32bit host code
mkoffload in principle handles 32bit and 64bit offload targets,
but 32bit support has no been implemented. Before this patch,
offloading is then silently disabled for the respective target.
With the patch, the user gets a warning by mkoffload (and the
programm continues to be build with out offloading code).
gcc/ChangeLog:
* config/gcn/mkoffload.cc (main): Warn for -foffload-abi=ilp32
that no offload code will be generated.
* config/nvptx/mkoffload.cc (main): Likewise.
libgomp/ChangeLog:
* testsuite/lib/libgomp-dg.exp (libgomp-dg-prune): Prune warning
by mkoffload that 32-bit offloading is not supported.
* testsuite/libgomp.c-c++-common/requires-1.c: Silence a FAIL for
'ia32' targets as for them no offload code is generated.
* testsuite/libgomp.c-c++-common/requires-3.c: Likewise.
* testsuite/libgomp.c-c++-common/requires-7.c: Likewise.
* testsuite/libgomp.c-c++-common/variable-not-offloaded.c: Likewise.
* testsuite/libgomp.fortran/requires-1.f90: Likewise.
gcc/config/gcn/mkoffload.cc| 5 -
gcc/config/nvptx/mkoffload.cc | 5 -
libgomp/testsuite/lib/libgomp-dg.exp | 3 +++
libgomp/testsuite/libgomp.c-c++-common/requires-1.c| 8 +---
libgomp/testsuite/libgomp.c-c++-common/requires-3.c| 8 +---
libgomp/testsuite/libgomp.c-c++-common/requires-7.c| 10 ++
.../testsuite/libgomp.c-c++-common/variable-not-offloaded.c| 4 ++--
libgomp/testsuite/libgomp.fortran/requires-1.f90 | 8 +---
8 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/gcc/config/gcn/mkoffload.cc b/gcc/config/gcn/mkoffload.cc
index 9a438de331a..c37c269d4d2 100644
--- a/gcc/config/gcn/mkoffload.cc
+++ b/gcc/config/gcn/mkoffload.cc
@@ -1143,7 +1143,10 @@ main (int argc, char **argv)
fatal_error (input_location, "cannot open %qs", gcn_cfile_name);
/* Currently, we only support offloading in 64-bit configurations. */
- if (offload_abi == OFFLOAD_ABI_LP64)
+ if (offload_abi == OFFLOAD_ABI_ILP32)
+warning (0, "offload code generation skipped: offloading with 32-bit host "
+ "code is currently not supported");
+ else if (offload_abi == OFFLOAD_ABI_LP64)
{
const char *mko_dumpbase = concat (dumppfx, ".mkoffload", NULL);
const char *hsaco_dumpbase = concat (dumppfx, ".mkoffload.hsaco", NULL);
diff --git a/gcc/config/nvptx/mkoffload.cc b/gcc/config/nvptx/mkoffload.cc
index 503b1abcefd..a7ff32cf8bd 100644
--- a/gcc/config/nvptx/mkoffload.cc
+++ b/gcc/config/nvptx/mkoffload.cc
@@ -798,7 +798,10 @@ main (int argc, char **argv)
/* PR libgomp/65099: Currently, we only support offloading in 64-bit
configurations. */
- if (offload_abi == OFFLOAD_ABI_LP64)
+ if (offload_abi == OFFLOAD_ABI_ILP32)
+warning (0, "offload code generation skipped: offloading with 32-bit host "
+ "code is currently not supported");
+ else if (offload_abi == OFFLOAD_ABI_LP64)
{
char *mko_dumpbase = concat (dumppfx, ".mkoffload", NULL);
if (save_temps)
diff --git a/libgomp/testsuite/lib/libgomp-dg.exp b/libgomp/testsuite/lib/libgomp-dg.exp
index ebf78e17e6d..9c9a5f2ed4b 100644
--- a/libgomp/testsuite/lib/libgomp-dg.exp
+++ b/libgomp/testsuite/lib/libgomp-dg.exp
@@ -3,5 +3,8 @@ proc libgomp-dg-test { prog do_what extra_tool_flags } {
}
proc libgomp-dg-prune { system text } {
+global additional_prunes
+
