[PATCH] D145579: [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 22 02:40:37 PDT 2023
awarzynski added a comment.
Really nice to see some shared code being elevated out of Clang into LLVM, thanks!
I've only reviewed on the Flang driver changes. I will let the OpenMP experts to review the remaining bits. All in all looks good, I've only made some small suggestions.
Thanks for working on this!
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:93
+std::string CodeGenAction::getAllTargetFeatures() {
+ std::string allFeaturesStr;
----------------
This method could be simplified by following https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code. For example:
```
std::string CodeGenAction::getAllTargetFeatures() {
if (!triple.isAMDGPU()) {
allFeaturesStr = llvm::join(targetOpts.featuresAsWritten.begin(),
targetOpts.featuresAsWritten.end(), ",");
return allFeaturesStr;
}
// The logic for AMDGPU
// Perhaps add a dedicated hook: getExplicitAndImplicitAMDGPUTargetFeatures()
}
```
Btw, this method does different things depending on the triple. Perhaps rename it as something more generic, e.g. `getTargetFeatures`? I think that the current name, `getAllTargetFeatures`, is a bit misleading (i.e. what does "all" mean?).
Also:
* make it `static`
* document
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145579/new/
https://reviews.llvm.org/D145579
More information about the llvm-commits
mailing list