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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates, mostly looks good. Just a couple of extra questions about the test coverage.



================
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
----------------
domada wrote:
> awarzynski wrote:
> > domada wrote:
> > > awarzynski wrote:
> > > > [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).
> > > I think that the main difference between Clang and Flang is the lack of `TargetInfo` class.
> > > 
> > > [[ https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html | TargetInfo classes ]] are Clang specific and they are responsible for parsing/adding default target features. Every target performs initialization in different way (compare for example [[ https://clang.llvm.org/doxygen/Basic_2Targets_2AArch64_8cpp_source.html#l00960 | AArch64 ]] vs [[ https://clang.llvm.org/doxygen/Basic_2Targets_2AMDGPU_8cpp_source.html#l00179 | AMDGPU ]] target initialization.
> > > 
> > > I don't want to make TargetInfo class Clang indendent (see discussion: https://discourse.llvm.org/t/rfc-targetinfo-library/64342 ). I also don't want to reimplement the whole TargetInfo class in Flang, because Flang already uses LLVM TargetMachine class (see: https://llvm.org/doxygen/classllvm_1_1TargetMachine.html and https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/FrontendActions.cpp#L614 )  which can play similar role as Clang TargetInfo IMO.
> > > 
> > > That's why I decided to implement `getExplicitAndImplicitAMDGPUTargetFeatures` function which performs initialization of default AMDGPU target features.
> > Thanks for this comprehensive overview! It would be helpful if this rationale was included in the summary (in the spirit of documenting things for our future selves).
> > 
> > So, there isn't anything special about AMDGPU here, is there? We will have to implement similar hooks for other targets at some point too, right? Or perhaps there's some reason to do this for AMDGPU sooner rather than later?
> > 
> > I'm not against this change, just want to better understand the wider context.
> Hi,
> I modified the comment above to be more informative.
> 
> IMO, we need to add similar hooks for other targets. For example:
> 
> ```
> clang --target=aarch64 t.c -S -emit-llvm -v 
> // I see in the logs explicit target features:
> // +neon,  +v8a ,  -fmv
> // However, generated t.ll contains 4 target features:
> "target-features"="+fp-armv8,+neon,+v8a,-fmv"
> // It looks like target feature +fp-armv8 is implicit
> ```
Thanks for checking!


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:147-151
+    llvm::SMDiagnostic err;
+    err.print(errorMsg.data(), llvm::errs());
+    unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "Unsupported feature ID");
+    ci.getDiagnostics().Report(diagID);
----------------



================
Comment at: flang/lib/Frontend/FrontendActions.cpp:149
+    err.print(errorMsg.data(), llvm::errs());
+    unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "Unsupported feature ID");
----------------
Are you able to add a test that will trigger this?


================
Comment at: flang/test/Driver/target-cpu-features.f90:59
+! CHECK-AMDGPU-R600: "-fc1" "-triple" "r600-unknown-unknown"
+! CHECK-AMDGPU-R600-SAME: "-target-cpu" "cayman"
----------------
Hm, there aren't any "implicit" target features here.


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

https://reviews.llvm.org/D145579