Re: [Patch][RFC] GCN: Define ISA archs in gcn-devices.def and use it

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Mar 15, 2024 at 5:36 PM Andrew Stubbs <ams@baylibre.com> wrote:
>
> On 15/03/2024 13:56, Tobias Burnus wrote:
> > Hi Andrew,
> >
> > Andrew Stubbs wrote:
> >> 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.
> >
> > Well, for .opt see how nvptx does it – it actually generates an .opt file.
> >
> >> I don't like the idea of including AMDGPU_ISA_UNSUPPORTED;
> >
> > I concur – I was initially thinking of reporting the device name
> > ("Unsupported %s") but I then realized that the agent returns a string
> > while only for GCC generated files (→ eflag) the hexcode is used. Thus,
> > I ended up not using it.
> >
> >> 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 was thinking of having more flags, but those where the only ones
> > required for the two files.
> >> 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.
> >
> > There is already:
> >
> > gcc/config/darwin-c.cc:#include "../../libcpp/internal.h"
> >
> > gcc/config/gcn/gcn-run.cc:#include
> > "../../../libgomp/config/gcn/libgomp-gcn.h"
> >
> > gcc/fortran/cpp.cc:#include "../../libcpp/internal.h"
> >
> > gcc/fortran/trigd_fe.inc:#include "../../libgfortran/intrinsics/trigd.inc"
> >
> > But there is also the reverse:
> >
> > libcpp/lex.cc:#include "../gcc/config/i386/cpuid.h"
> >
> > libgfortran/libgfortran.h:#include "../gcc/fortran/libgfortran.h"
> >
> > lto-plugin/lto-plugin.c:#include "../gcc/lto/common.h"
> >
> > If you add more items, it is probably better to have it under
> > gcc/config/gcn/ - and I really prefer a single file for all.
> >
> > * * *
> >
> > Talking about feature sets: This would be a bit like LLVM (see below)
> > but I think they have a bit too much indirections. But I do concur that
> > we need to consolidate the current support – and hopefully make it
> > easier to keep adding more GPU support; we seem to have already covered
> > a larger chunk :-)
> >
> > I also did wonder whether we should support, e.g., running a gfx1100
> > code (or a gfx11-generic one) on, e.g., a gfx1103 device. Alternatively,
> > we could keep the current check which requires an exact match.
>
> We didn't invent that restriction; the runtime won't let you do it. We
> only have the check because the HSA/ROCr error message is not very
> user-friendly.

Note that I heard/read somewhere that they plan to support a "blend" version
that would allow running a kernel on any gfx11xx or gfx106x versions (supposedly
at some runtime cost).  Guess we'll need to watch the LLVM side of things here
(or the ROCm runtime side of it).

> > BTW: I do note that looking at the feature sets in LLVM that all GFX110x
> > GPUs seem to have common silicon bugs: FeatureMSAALoadDstSelBug and
> > FeatureMADIntraFwdBug, while 1100 and 1102 additionally have the
> > FeatureUserSGPRInit16Bug but 1101 and 1103 don't. — For some reasons,
> > FeatureISAVersion11_Generic only consists of two of those bugs (it
> > doesn't have FeatureMADIntraFwdBug), which doesn't seem to be that
> > consistent. Maybe the workaround has issues elsewhere? If so, a generic
> > -march=gfx11 might be not as useful as one might hope for.
> >
> > * * *
> >
> > If I look at LLVM's
> > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPU.td ,
> >
> > they first define several features – like 'FeatureUnalignedScratchAccess'.
> >
> > Then they combine them like in:
> >
> > def FeatureISAVersion11_Common ... [FeatureGFX11, ...
> > FeatureAtomicFaddRtnInsts ...
> >
> > And then they use those to map them to feature sets like:
> >
> > def FeatureISAVersion11_0_Common ...
> > listconcat(FeatureISAVersion11_Common.Features,
> >      [FeatureMSAALoadDstSelBug ...
> >
> > And for gfx1103:
> >
> > def FeatureISAVersion11_0_3 : FeatureSet<
> >    !listconcat(FeatureISAVersion11_0_Common.Features,
> >      [])>;
> >
> > The mapping to gfx... names then happens in
> > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNProcessors.td such as:
> >
> > def : ProcessorModel<"gfx1103", GFX11SpeedModel,
> >    FeatureISAVersion11_0_3.Features
> >  >;
> >
> > Or for the generic one, i.e.:
> >
> > // [gfx1100, gfx1101, gfx1102, gfx1103, gfx1150, gfx1151]
> > def : ProcessorModel<"gfx11-generic", GFX11SpeedModel,
> >    FeatureISAVersion11_Generic.Features
> >
> > LLVM also has some generic flags like the following in
> > https://github.com/llvm/llvm-project/blob/main/llvm/lib/TargetParser/TargetParser.cpp
> >
> >      {{"gfx1013"},   {"gfx1013"}, GK_GFX1013,
> > FEATURE_FAST_FMA_F32|FEATURE_FAST_DENORMAL_F32|FEATURE_WAVE32|FEATURE_XNACK|FEATURE_WGP},
> >
> > I hope that this will give some inspiration – but I assume that at least
> > the initial implementation will be much shorter.
>
> Yeah, we can have one macro for each arch, or multiple macros for
> building different tables. First one seems easier but less readable,
> second one will need some thinking about. Probably best to keep it
> simple though.

I'd say whatever gets us to smaller required changes to say introduce support
for gfx1013 is welcome.  Applies to the libgomp plugin side as well, of course.

Richard.

> Andrew