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: > 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 ``` 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
