[PATCH] D60620: [HIP] Support target id by --offload-arch

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D60620#2089722 <https://reviews.llvm.org/D60620#2089722>, @yaxunl wrote:

> In D60620#2067134 <https://reviews.llvm.org/D60620#2067134>, @tra wrote:
>
> > Do you expect users to specify these IDs? How do you see it being used in practice? I think you do need to implement a user-friendly shortcut and expand it to the detailed offload-id internally. I'm fine with allowing explicit offload id as a hidden argument, but I don't think it's suitable for something that will be used by everyone who can't be expected to be aware of all the gory details of particular GPU features.
>
>
> The good thing about this target id is that it is backward compatible with GPU arch. For common users who are not concerned with specific GPU configurations, they can just use the old GPU arch and nothing changes. This is because GPU arch without features implies default value for these features, which work on all configurations. For advanced users who do need to build for specific GPU configurations, they should already have the knowledge about the name and meaning of these configurations by reading the AMDGPU user guide (http://llvm.org/docs/AMDGPUUsage.html). Therefore a target id in the form of gfx908:xnack+ is not something cryptic to them. On the other hand, an encoded GPU arch like gfx908a is cryptic since it has no meaning at all.


I don't quite agree with the `gfx908:xnack+ is not something cryptic` assertion. I've looked at the AMDGPUUsage.html and I am pretty sure that I still have no clue which ID will be correct for my WX8200. It does not mention the card, nor does it specify the offload format. Having to type the IDs with the features ordered just so (i.e. without normalization) puts a fair amount of burden on the user. Not only they must remember which features must be on or off, but they also need to specify them in a very specific order (it's not even lexicographically ordered) . I think adding normalization to make it possible to specify features in arbitrary order would mitigate some of it.

As it's implemented now, my bet is that it will be *very* annoying to use in practice.

At the very least,  you should document the requirements for the offload ID format with the specific examples. It would also be useful to provide specific offload IDs for particular GPU cards as that's what regular users will have info about. Right now the AMDGPUUsage doc does not provide sufficient details to derive correct offload ID if all you have is a name of the GPU card. That's going to be the case for most of clang users who just want to build things for their GPU.

That said, the scheme in the current version of the patch is flexible enough to retrofit simplified names later, so I'm overall OK with proceeding with the patch once documentation has been updated.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60620/new/

https://reviews.llvm.org/D60620