[PATCH] D145579: [Flang][AMDGPU] Add support for AMDGPU to Flang driver
Dominik Adamski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 22 06:29:41 PDT 2023
domada marked an inline comment as done.
domada added inline comments.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:93
+std::string CodeGenAction::getAllTargetFeatures() {
+ std::string allFeaturesStr;
----------------
awarzynski wrote:
> 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
Hi,
thanks for the review. I applied your remarks. I also moved OpenMP related changes to the child review.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145579/new/
https://reviews.llvm.org/D145579
More information about the llvm-commits
mailing list