Hi!
On 2024-04-25T16:07:53+0100, Andrew Stubbs <a...@baylibre.com> 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
