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

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

Couple of minor nits. LGTM otherwise.



================
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>
----------------
yaxunl wrote:
> tra wrote:
> > Comment needs updating as parameters and return value have changed.
> done
The comment still mentions `\p IsValid`.


================
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);
----------------
yaxunl wrote:
> tra wrote:
> > Looks like a good candidate for using a std::optional<std::pair> return value.
> > 
> done
`hasConflictingTargetIDCombination()` ? Optional is convertible to bool and `has` better reflects the purpose of the function -- you want to know whether there's a conflict. What exactly conflicts is sort of secondary info, only used to provide additional details for diags.


================
Comment at: clang/lib/Basic/TargetID.cpp:161
+      if (llvm::any_of(Features, [&](auto &F) {
+            return ExistingFeatures.find(F.first()) == ExistingFeatures.end();
+          }))
----------------
Nit: `find(...) == end()` -> `count == 0` ? Makes it shorter and arguably easier to read.


================
Comment at: clang/lib/Driver/Driver.cpp:2795-2799
+      auto CTID = getConflictTargetIDCombination(GpuArchs);
+      if (!CTID)
+        return true;
+      ConflictingTIDs = CTID.getValue();
+      return false;
----------------
Could be simplified a bit:
```
if (auto CTID = getConflictTargetIDCombination(GpuArchs)) {
  ConflictingTIDs = CTID.getValue();
  return false
}
return true;
```

Also, it does not seem to add any new functionality to getConflictTargetIDCombination(). Perhaps it would make sense to change the function signatures to match and just use `return getConflictTargetIDCombination()`.


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

https://reviews.llvm.org/D60620