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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia 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
----------------
jansvoboda11 wrote:
> 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?
Sorry for the delay.

> My understanding is that -cc1 is an internal interface, so end-users should never use -cc1 directly and/or rely on itss interface. 

This is true in practice but there are developers that use `-cc1` too. My main concern is however that the internal testing gets more complicated - with so many more flags to be added that can also be easy to miss.

Is there any compromise we could find?  


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

https://reviews.llvm.org/D82756