jansvoboda11 added inline comments.
================ Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2 +// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only -menable-unsafe-fp-math \ +// RUN: -menable-no-infs -menable-no-nans -fno-signed-zeros -freciprocal-math \ +// RUN: -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | FileCheck %s ---------------- dang wrote: > Anastasia wrote: > > dang wrote: > > > Anastasia wrote: > > > > dang wrote: > > > > > Anastasia wrote: > > > > > > Not clear why do you need to pass these extra flags now? > > > > > Previously passing -ffast-math to CC1 implied all these other flags. > > > > > I am trying to make CC1 option parsing as simple as possible, so that > > > > > we can then make it easy to generate a command line from a > > > > > CompilerInvocation instance. You can refer to [[ > > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | > > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for > > > > > more details on why we want to be able to do this > > > > Just to understand, there used to be implied flags and it made the > > > > manual command line use of clang more compact and easy... Is the idea > > > > now to change those compound flags such that individul flags always > > > > need to be passed? > > > > > > > > Although I thought you are still adding the implicit flags: > > > > > > > > {options::OPT_cl_fast_relaxed_math, > > > > [&](const Arg *Arg) { > > > > RenderArg(Arg); > > > > > > > > > > > > CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable)); > > > > CmdArgs.push_back(GetArgString(options::OPT_ffast_math)); > > > > > > > > CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only)); > > > > CmdArgs.push_back( > > > > GetArgString(options::OPT_menable_unsafe_fp_math)); > > > > CmdArgs.push_back(GetArgString(options::OPT_mreassociate)); > > > > > > > > CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans)); > > > > CmdArgs.push_back( > > > > GetArgString(options::OPT_menable_no_infinities)); > > > > > > > > CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros)); > > > > > > > > CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math)); > > > > CmdArgs.push_back(GetArgString(options::OPT_fapprox_func)); > > > > }} > > > > > > > > Do I just misunderstand something? > > > The command line of the driver doesn't change. This patch only affects > > > what CC1 understands, now CC1 doesn't know anymore that > > > `-cl-fast-relaxed-math` implies all these other options so the driver is > > > responsible for specifying them when it constructs the CC1 command line. > > > > > > To summarize, the clang driver command line isn't affected by this patch > > > and it shouldn't be so let me know if something is wrong there. However, > > > manually constructed `clang -cc1` invocations need to specify the all the > > > implied flags manually now. > > Yes I understand, however, I am wondering whether this is intuitive because > > it seems the behavior of clang with `-cc1` and without will be different if > > the same `-cl-fast-relaxed-math` flag is passed. > > > > I also find adding all the flags manually is too verbode if > > `-cl-fast-relaxed-math` assumes to enable all the extra setting. > My understanding is that `-cc1` is an internal interface, so end-users should > never use `-cc1` directly and/or rely on itss interface. It is already the > case that flags mean very different things to the driver and `-cc1` for > example "--target=" and "-triple". Furthermore, this impacted very few tests > which leads me to believe that few compiler developers actually rely on this > behavior. > > Do you think this would be a major inconvenience to compiler developers to > have to manually expand it out? Hi @Anastasia, I'll be taking over this patch. I agree with Daniel that `-cc1` is an internal interface that doesn't need to match the public driver interface. The current approach is by far the simplest to get command-line option marshaling working. What are your thoughts? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82756/new/ https://reviews.llvm.org/D82756 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
