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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
