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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
