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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: clang/include/clang/Driver/Driver.h:332
 
+  llvm::Triple getHIPOffloadTargetTriple() const;
+
----------------
This is used exclusively by the Driver.cpp and does not have to be a public API call.


================
Comment at: clang/lib/Basic/TargetID.cpp:18
+
+static const llvm::SmallVector<llvm::StringRef, 4>
+getAllPossibleAMDGPUTargetIDFeatures(const llvm::Triple &T,
----------------
Nit. You could use llvm::SmallVectorImpl<llvm::StringRef> -- caller only cares that it's an array of StringRef and does not need to know the size hint.
Makes it easier to change the hint w/o having to replace the constant evrywhere.


================
Comment at: clang/lib/Basic/TargetID.cpp:53
+
+llvm::StringRef parseTargetID(const llvm::Triple &T,
+                              llvm::StringRef OffloadArch,
----------------
A comment describing expected format would be helpful.



================
Comment at: clang/lib/Basic/TargetID.cpp:53-69
+llvm::StringRef parseTargetID(const llvm::Triple &T,
+                              llvm::StringRef OffloadArch,
+                              llvm::StringMap<bool> *FeatureMap,
+                              bool *IsValid) {
+  llvm::StringRef ArchStr;
+  auto SetValid = [&](bool Valid) {
+    if (IsValid)
----------------
tra wrote:
> A comment describing expected format would be helpful.
> 
I'd restructure things a bit.
First, I'd make return type std::optional<StringRef>and fold IsValid into it.
Then I would make FeatureMap argument a non-optional, so the parsing can concentrate on parsing only.
Then I'd add another overload without FeatureMap argument, which would be a warpper over the real parser with a temp FeatureMap which will be discarded.

This should make things easier to read.


================
Comment at: clang/lib/Basic/TargetID.cpp:103
+
+std::string getCanonicalTargetID(llvm::StringRef Processor,
+                                 const llvm::StringMap<bool> &Features) {
----------------
What does 'canonical' mean? A comment would be helpful.


================
Comment at: clang/lib/Basic/TargetID.cpp:116
+static llvm::StringRef
+parseCanonicalTargetIDWithoutCheck(llvm::StringRef OffloadArch,
+                                   llvm::StringMap<bool> *FeatureMap) {
----------------
Perhaps we can further split parsing offloadID vs checking whether it's valid and make parseTargetID above call this parse-only helper.

E.g. something like this:

```
something parseTargetIDhelper(something); // Parses targetID 
something isTargetIdValid(something);      // Verivies validity of parsed parts.
std::optional<StringRef> parseTargetID(FeatureMap) {
   parseTargetIDhelper(...);
   if (!targetIDValid())
      return None;
   return Good;
}
std::optional<StringRef> parseTargetID() {
   auto TempFeatureMap;
  return parseTargetID(&TempFeatureMap);
}
```


================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:366
+          std::replace(NewF.begin(), NewF.end(), '-', '_');
+          Builder.defineMacro(Twine("__amdgcn_") + Twine(NewF) + Twine("__"),
+                              Loc->second ? "1" : "0");
----------------
Nit: Should it be "__amdgcn_feature_" to make it more explicit where these macros are derived from?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:601-605
+        llvm::MDString::get(
+            getModule().getContext(),
+            TargetIDStr == ""
+                ? TargetIDStr
+                : (Twine(getTriple().str()) + "-" + TargetIDStr).str()));
----------------
I think this may cause problems.

Twine.str() will return a temporary std::string.
MDString::get takes a StringRef as the second parameter, so it will be a reference to the temporary. It will then get added to the module's metadata which will probably outlive the temporary string. The tests for the MDString do appear to use string storage that outlives MDString.



================
Comment at: clang/lib/Driver/Driver.cpp:2602-2603
 
+      if (!isValidOffloadArchCombination(GpuArchs))
+        return true;
+
----------------
This is something we may want to diagnose. 


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

https://reviews.llvm.org/D60620