[PATCH] D145579: [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect

[Andrzej Warzynski via Phabricator via llvm-commits]

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