domada marked an inline comment as done. domada added inline comments.
================ 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 ---------------- 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. 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
