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

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked 20 inline comments as done.
yaxunl added inline comments.


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


================
Comment at: clang/lib/Basic/TargetID.cpp:18
+
+static const llvm::SmallVector<llvm::StringRef, 4>
+getAllPossibleAMDGPUTargetIDFeatures(const llvm::Triple &T,
----------------
tra wrote:
> 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.
It seems I cannot return a SmallVector as SmallVectorImpl since copy ctor is deleted.


================
Comment at: clang/lib/Basic/TargetID.cpp:53
+
+llvm::StringRef parseTargetID(const llvm::Triple &T,
+                              llvm::StringRef OffloadArch,
----------------
tra wrote:
> 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.
done


================
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)
----------------
yaxunl wrote:
> tra wrote:
> > 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.
> done
parseTargetID actually has two usage pattern: 1. parse the entire target ID including processor and features and returns the processor, features, and whether the target ID is valid 2. parse the processor part of the target ID only and returns the processor or an empty string if the processor is invalid

For usage 1 I will revise it by your suggestion.

For usage 2 I will separate it to a different function getProcessorFromTargetID


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


================
Comment at: clang/lib/Basic/TargetID.cpp:116
+static llvm::StringRef
+parseCanonicalTargetIDWithoutCheck(llvm::StringRef OffloadArch,
+                                   llvm::StringMap<bool> *FeatureMap) {
----------------
tra wrote:
> 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);
> }
> ```
done


================
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");
----------------
tra wrote:
> Nit: Should it be "__amdgcn_feature_" to make it more explicit where these macros are derived from?
done


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:601-605
+        llvm::MDString::get(
+            getModule().getContext(),
+            TargetIDStr == ""
+                ? TargetIDStr
+                : (Twine(getTriple().str()) + "-" + TargetIDStr).str()));
----------------
tra wrote:
> 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.
> 
I checked the implementation of MDString::get. It seems to create its own copy of the string in a StringMap and use it.


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


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

https://reviews.llvm.org/D60620