MaskRay added a comment. In D89799#2345004 <https://reviews.llvm.org/D89799#2345004>, @awarzynski wrote:
> Thank you all for you comments! Please find my replies below. I've picked 4 > main points raised here. > > 1 > - > > In D89799#2342677 <https://reviews.llvm.org/D89799#2342677>, @rnk wrote: > >> This seems like pretty corner case functionality. Do we really need this >> diagnostic? > > Good point. If nobody objects I will remove it. +1 for removing it. > 2 > - > > In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote: > >> I am not sure whether it is proper to rename it. >> >> Originally, this flag means driver option which is not supposed to be >> forwarded to tools. It is more like a reminder to driver developers since >> clang driver does not automatically forward options to tools and does not >> enforce not forwarding options with this flag to tools. > > I'm happy to rename this differently, e.g. `DontForwardToTools`. However, > `DriverOption` is very confusing to me. Which driver is this flag referring > to? Compiler driver (`clang`) ? Frontend driver (`clang -cc1`)? If it refers > to libclangDriver in general, then everything in `Options.td` is a driver > flag by default (i.e. it belongs to the library the implements the compiler > driver API). Also, based on the current usage and feedback from [1]: > >> It served two purposes (1) (@awarzynski: No longer applies) (2) whether an >> option should be forwarded for -Xarch -Xarch_host -Xarch_device (macOS and >> CUDA use cases) > > I feel that `DriverOption` is currently too generic. The original purposes have mostly been eliminated. The remaining is now -Xarch. `DontForwardToTools` does not sound useful to me. Very few tools (cc1/assembler/linker) accept driver options and they should just take an allowlist instead of requiring to annotate the opposite. > 3 > - > > In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote: > >> Then some toolchains use this flag as a heuristic not to use options with >> this flag with -Xarch. For that purpose it is too broad as many options with >> this flag can be used with -Xarch. > > Isn't there a more broader problem with various flags being used incorrectly > in Options.td? I would love to help improving that, but I'm new to this > codebase. Are there any heuristics that I could use to guide me in that? For > example - how do I decide where to delete `DriverOption` from? > Btw, are you suggesting that this change will be incompatible with some > downstream projects? > > 4 > - > > In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote: > >> So the question is: Is there any value to keep the original intention of >> this flag, i.e. mark some options as driver options without enforcing it? Or >> do we want to add assertions or warnings in clang -cc1 to check if driver >> options are passed to FE? > > Isn't this already enforced in `ToolChain::TranslateXarchArgs` (via the > diagnostic)? That's the only place where `DriverOption` is currently used. > Also, FE? > > Thank you for reviewing! > [1] http://lists.llvm.org/pipermail/cfe-dev/2020-October/067023.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89799/new/ https://reviews.llvm.org/D89799 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
