[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang 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
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82756/new/

https://reviews.llvm.org/D82756