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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Looks good in general. Mostly C++ style comments below.



================
Comment at: clang/include/clang/Basic/TargetID.h:30
+/// Returns canonical processor name or empty if the processor name is invalid.
+llvm::StringRef getProcessorFromTargetID(const llvm::Triple &T,
+                                         llvm::StringRef OffloadArch);
----------------
Nit: In cases where performance is not absolutely critical, I'd prefer to use std::string. This way I don't need to worry what exactly is that reference referencing and I can just store the result. Keeps things simple. With StringRef one has to be more cautious -- how long will that reference keep pointing to the right value? In general, the answer requires knowing the details of the implementation. With std::string, you just use the value and let compiler eliminate intermediate values.
In this case you have used StringRef in other places and it's also used for similar purposes all over the place, so it's just my personal preference. 


================
Comment at: clang/include/clang/Basic/TargetID.h:42
+/// is not a null pointer.
+/// If \p CanonicalizeProc is true, canonicalize returned processor name.
+llvm::Optional<llvm::StringRef>
----------------
Comment needs updating as parameters and return value have changed.


================
Comment at: clang/include/clang/Basic/TargetID.h:56-58
+bool isValidTargetIDCombination(
+    const std::set<llvm::StringRef> &TargetIDs,
+    std::pair<llvm::StringRef, llvm::StringRef> *ConflictingTIDs = nullptr);
----------------
Looks like a good candidate for using a std::optional<std::pair> return value.



================
Comment at: clang/lib/Basic/TargetID.cpp:161-169
+      for (auto &&F : Features) {
+        if (ExistingFeatures.find(F.first()) == ExistingFeatures.end()) {
+          if (ConflictingTIDs) {
+            ConflictingTIDs->first = Loc->second.TargetID;
+            ConflictingTIDs->second = ID;
+          }
+          return false;
----------------
This could probably be expressed better with any_of():
```
if (llvm::any_of(Features, [](auto &F){
   return ExistingFeatures.count(F.first) == 0;
  })
  return {Loc->second.TargetID, ID};
```

The outer loop could also be transformed into a form of llvm::for_each or llvm::any_of() with an inner lambda returning an optional tuple on conflict.



================
Comment at: clang/lib/Basic/Targets/AMDGPU.h:404
+  // pre-defined macros.
+  bool handleTargetFeatures(std::vector<std::string> &Features,
+                            DiagnosticsEngine &Diags) override {
----------------
We never return anything but true. Change return to void?


================
Comment at: clang/lib/Basic/Targets/AMDGPU.h:412-417
+      auto Loc =
+          std::find(TargetIDFeatures.begin(), TargetIDFeatures.end(), Name);
+      if (Loc == TargetIDFeatures.end())
+        continue;
+      assert(OffloadArchFeatures.find(Name) == OffloadArchFeatures.end());
+      OffloadArchFeatures[Name] = IsOn;
----------------
Nit: for small-ish loops over ranges, I generally find that standard functional-stile functions to be more expressive. 
IMO, it's easier to read something like this:

```
llvm::for_each(Features, [](auto F){
    ...
   Name = ...
   if (llvm::any_of(TargetIDFeatures, [](N){ return N == Name; })) { // or use llvm::find()
      // update OffloadArchFeatures.
   }
})
```

Again, it's a personal style choice. The function is OK as is, I'm just flagging places where I had to think what the code does, where the code could convey the intent in a more direct way.


================
Comment at: clang/lib/Driver/Driver.cpp:98-99
+static llvm::Triple getHIPOffloadTargetTriple() {
+  static const llvm::Triple T("amdgcn-amd-amdhsa");
+  return T;
+}
----------------
Why not just return llvmTriple("amdgcn-amd-amdhsa") ?


================
Comment at: clang/lib/Driver/Driver.cpp:2403
+      /// Target ID string which is persistent throughout the compilation.
+      const char *ID;
+      TargetID(CudaArch Arch) { ID = CudaArchToString(Arch); }
----------------
just make it std::string. There's no point tinkering with pointers here.

Also, I'm not sure why the whole TargetID can't be just a std::string.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:523
+  StringRef GpuArch;
+  llvm::StringMap<bool> FeatureMap;
+  StringRef TargetID = DriverArgs.getLastArgValue(options::OPT_mcpu_EQ);
----------------
I'd move both vars  down to where they are used first.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:526
+  if (TargetID.empty())
+    return GpuArch;
+
----------------
`StringRef()` would make it more explicit that it's a failure.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531
+    getDriver().Diag(clang::diag::err_drv_bad_target_id) << TargetID;
+    return GpuArch;
+  }
----------------
ditto.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:543-545
+    auto Pos = FeatureMap.find(Feature);
+    if (Pos == FeatureMap.end())
+      continue;
----------------
`FeatureMap.count() == 0` ? 


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:547-548
+    CC1Args.push_back("-target-feature");
+    auto FeatureName = Feature;
+    std::string Opt = (Twine(Pos->second ? "+" : "-") + FeatureName).str();
+    CC1Args.push_back(DriverArgs.MakeArgStringRef(Opt));
----------------
Do you need this variable? It appears to be used only once. Maybe just fold everything into MakeArgStringRef, if it does not get too unreadable?


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

https://reviews.llvm.org/D60620