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