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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

A few more comments, but mostly nits. Btw, is this patch sufficient to generate code for AMDGPU? Or, put differently, what's the level of support atm?



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:107
     break;
+  case llvm::Triple::r600:
+  case llvm::Triple::amdgcn:
----------------
Should there be a test for this triple as well?


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:134-137
+  if (!triple.isAMDGPU()) {
+    return llvm::join(targetOpts.featuresAsWritten.begin(),
+                      targetOpts.featuresAsWritten.end(), ",");
+  }
----------------
[nit] I would probably flip this as:
```
if (triple.isAMDGPU()) {
  // Clang does not append all target features to the clang -cc1 invocation.
  // Some AMDGPU features are passed implicitly by the Clang frontend.
  // That's why we need to extract implicit AMDGPU target features and add
  // them to the target features specified by the user
  return getExplicitAndImplicitAMDGPUTargetFeatures(ci, targetOpts, triple);
  }
``` 

I know that I suggested the opposite in my previous comment, but you have simplified this since :) In any case, feel free to ignore.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:139-142
+  // Clang does not append all target features to the clang -cc1 invocation.
+  // Some AMDGPU features are passed implicitly by the Clang frontend.
+  // That's why we need to extract implicit AMDGPU target features and add
+  // them to the target features specified by the user
----------------
[nit] IMHO this is documenting an implementation detail that would be more relevant inside `getExplicitAndImplicitAMDGPUTargetFeatures`.

More importantly, you are suggesting that the driver is doing whatever it is doing "because that's what Clang does". Consistency with Clang is important (you could call it out in the commit message) :) However, it would be even nicer to understand the actual rationale behind these implicit features. Any idea? Perhaps there are some clues in git history?

Also, perhaps a TODO to share this code with Clang? (to make it even clearer that the frontend driver aims for full consistency with Clang here).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145579/new/

https://reviews.llvm.org/D145579