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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/HIP.cpp:121-123
+  auto Pos = SubArchName.find_first_of("+-");
+  if (Pos != SubArchName.npos)
+    SubArchName = SubArchName.substr(0, Pos);
----------------
tra wrote:
> Parsing should probably be extracted into a separate function to avoid replicating it all over the place.
> 
> I'd also propose use a different syntax for the properties.
> * use explicit character to separate individual elements. This way splitting the properties becomes independent of what those properties are. If you decide to make properties with values or change their meaning some other way, it would not affect how you compose them.
> * use `name=value` or `name[+-]` for individual properties. This makes it easy to parse individual properties and normalize their names. This makes property map creation independent of the property values.
> 
> Right now `[+-]` serves as both a separator and as the value, which would present problems if you ever need more flexible parametrization of properties. What if a property must be a number or a string. Granted, you can always encode them as a set of bools, but that's rather unpractical. 
> 
> E.g. something like this would work a bit better: `gfx111:foo+:bar=33:buz=string`.
> 
I discussed this with our team.

The target id features are not raw GPU properties. They are distilled to become a few target features to decide what the compiler should do.

Each target feature has 3 values: on, off, and default, which are encoded as +feature, -feature, and not present.

For runtime, it is simple and clear how to choose device binaries based on the target features: it will try exact match, otherwise choose the default.

For compiler, it is also simple and clear what to do for each target feature value, since they corresponding to backend target features.

Basically we expect the target id feature to be like flags, not key/value pairs. In case we do need key/value pairs, they can still use + as delimiter.

Another reason we use +/- format is that it is more in line with the format of existing clang-offload-bundler id and target triple, which uses - as delimiter.

Since the target id is an extension of offload arch and users will put it into command line, we want to make it short, concise and aesthetically appealing, we would avoid using non-alpha-numeric characters in the target id features. Target triple components have similar requirements. Using : as delimiter seems unnecessary, longer, and more difficult to read.

Consider the following example


```
clang -offload-id gfx908+xnack-sramecc a.hip

clang -offload-id gfx908:xnack+:sramecc- a.hip
```

We are more inclined to keep the original format. 


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

https://reviews.llvm.org/D60620