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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
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>
----------------
tra wrote:
> Comment needs updating as parameters and return value have changed.
done


================
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);
----------------
tra wrote:
> Looks like a good candidate for using a std::optional<std::pair> return value.
> 
done


================
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;
----------------
tra wrote:
> 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.
> 
will only change the inner loop since changing the outer loop makes the code more difficult to understand.


================
Comment at: clang/lib/Basic/Targets/AMDGPU.h:404
+  // pre-defined macros.
+  bool handleTargetFeatures(std::vector<std::string> &Features,
+                            DiagnosticsEngine &Diags) override {
----------------
tra wrote:
> We never return anything but true. Change return to void?
This is a target hook which allows target specific handling. Some targets may return false.


================
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;
----------------
tra wrote:
> 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.
done


================
Comment at: clang/lib/Driver/Driver.cpp:98-99
+static llvm::Triple getHIPOffloadTargetTriple() {
+  static const llvm::Triple T("amdgcn-amd-amdhsa");
+  return T;
+}
----------------
tra wrote:
> Why not just return llvmTriple("amdgcn-amd-amdhsa") ?
to avoid construct this multiple times and have multiple copies


================
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); }
----------------
tra wrote:
> 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.
This is used by both CUDA and HIP. For CUDA it is the GPU arch string, for HIP it is target ID. The const char* passed to the ctor is persistent through the whole compilation already. And their usage expect them to be persistent across the whole compilation. Changing this to std::string make it not persist across the whole compilation since it is a member of ActionBuilder.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:543-545
+    auto Pos = FeatureMap.find(Feature);
+    if (Pos == FeatureMap.end())
+      continue;
----------------
tra wrote:
> `FeatureMap.count() == 0` ? 
we need to use Pos below


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

https://reviews.llvm.org/D60620