On 15/03/2024 12:21, Tobias Burnus wrote:
Given the large number of AMD GPU ISAs and the number of files which
have to be adapted, I wonder whether it makes sense to consolidate this
a bit, especially in the light that we may want to support more in the
future.
Besides using some macros, I also improved the diagnostic if the object
code couldn't be recognized (shouldn't happen) or if the GPU is
unsupported (likely; it now prints the GPU string). I was initially
thinking of resolving the arch encoded in the eflag to a string, but as
this is about GCC-generated code, it seemed to be unlikely of much use.
[It should that rare that we might also go back to the static string
instead of outputting the hex value of the eflag.]
Note: I only modified mkoffload.cc and plugin-gcn.c, but with some
tweaks it could also be used for other files in gcc/config/gcn/.
If you add a new ISA, you still need to update plugin-gcn.c's
max_isa_vgprs and the xnack/sram-ecc handling in mkoffload.c's main, but
that should be all for those two files.
Thoughts?
This is more-or-less what I was planning to do myself, but as I want to
include all the other features that get parametrized in gcn.cc, gcn.h,
gcn-hsa.h, gcn-opts.h, I hadn't got around to it yet. Unfortunately, I
think the gcn.opt and config.gcc will always need manually updating, but
if that's all it'll be an improvement.
I don't like the idea of including AMDGPU_ISA_UNSUPPORTED; that list is
going to be permanently out of date, and even if we maintain it
fastidiously last year's release isn't going to have the updated list in
the wild. I think it's not actually active in this patch in any case.
Instead of AMDGPU_ISA, I think "AMDGPU_ELF" makes more sense. The ISA is
"CDNA2" or "RDNA3", etc., and the compiler needs to know about that.
Ultimately, I want to replace many of the conditionals like
"TARGET_CDNA2_PLUS" from the code and replace them with feature flags
derived from a def file, or at least a header file. We've acquired too
many places where there are unsearchable conditionals that need finding
and fixing every time a new device comes along.
I had imagined that this .def file would exist in gcc/config/gcn, but
you've placed it in libgomp.... maybe it makes sense to have multiple
such files if they contain very different data, but I had imagined one
file and I'm not sure that the compiler definitions live in libgomp.
Tobias
PS: I think the patch is fine and builds, but I have not tested it on an
AMD GPU machine, yet.
PPS: For using for other files, see also in config/nvptx which uses
nvptx-sm.def to generate several files.
Andrew
