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

[Andrew Stubbs <ams@baylibre.com>]

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.

> 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.

Andrew