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