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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
