[PATCH] D145579: [Flang][AMDGPU] Add support for AMDGPU to Flang driver

[Dominik Adamski via Phabricator via cfe-commits]

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