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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Basic/HIP.cpp:16
+const llvm::SmallVector<llvm::StringRef, 4>
+getAllPossibleTargetIdFeatures(llvm::StringRef Device) {
+  llvm::SmallVector<llvm::StringRef, 4> Ret;
----------------
Nit: there's an unfortunate clash with already [[ https://github.com/llvm/llvm-project/blob/6a3469f58d0c230e86043f6975f048968334dfa4/clang/include/clang/Driver/CC1Options.td#L23 | target-feature ]] in clang & LLVM.

Would something like `GPUProperties` be a reasonable name?


================
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);
----------------
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`.



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

https://reviews.llvm.org/D60620