dexonsmith accepted this revision.
dexonsmith added a comment.
LGTM. I've just done a careful audit myself, and I'm now confident this patch
is correct and that there is no latent bug -- that it's correct to ignore
`-f*trapping-math` on the `-cc1` command-line since `-fp-exception-mode` will
always have a value matching / superseding `-f*trapping-math`.
@SjoerdMeijer , please confirm you agree as well.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2598-2601
FPExceptionBehavior = "strict";
FPModel = Val;
FPContract = "off";
TrappingMath = true;
----------------
`FPExceptionBehavior` and `TrappingMath` are set together here (in lock step).
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2626-2647
case options::OPT_ftrapping_math:
if (!TrappingMathPresent && !FPExceptionBehavior.empty() &&
!FPExceptionBehavior.equals("strict"))
// Warn that previous value of option is overridden.
D.Diag(clang::diag::warn_drv_overriding_flag_option)
<< Args.MakeArgString("-ffp-exception-behavior=" +
FPExceptionBehavior)
<< "-ftrapping-math";
----------------
Note this handling of `-ftrapping-math` in the driver, which uses it to set
`FPExceptionBehaviour` correctly.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2698-2717
// Validate and pass through -ffp-exception-behavior option.
case options::OPT_ffp_exception_behavior_EQ: {
StringRef Val = A->getValue();
if (!TrappingMathPresent && !FPExceptionBehavior.empty() &&
!FPExceptionBehavior.equals(Val))
// Warn that previous value of option is overridden.
D.Diag(clang::diag::warn_drv_overriding_flag_option)
----------------
Note that `-ffp-exception-behaviour` also sets `FPExceptionBehaviour` (and
`TrappingMath`).
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2732-2733
SignedZeros = false;
TrappingMath = false;
FPExceptionBehavior = "";
break;
----------------
This will override `FPExceptionBehavior`, (wiping out any effect of
`-ftrapping-math`), but that seems intentional and `TrappingMath` is set the
same way.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2739-2740
SignedZeros = true;
TrappingMath = true;
FPExceptionBehavior = "strict";
----------------
`FPExceptionBehavior` and `TrappingMath` are also set together here.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2829-2832
if (TrappingMath) {
// FP Exception Behavior is also set to strict
assert(FPExceptionBehavior.equals("strict"));
+ }
----------------
Note: given the above, this assertion seems sufficient for confirming
`-ftrapping-math` is effectively passed to `-cc1`.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3068-3075
- if (Args.hasArg(OPT_ftrapping_math)) {
- Opts.setFPExceptionMode(LangOptions::FPE_Strict);
- }
-
- if (Args.hasArg(OPT_fno_trapping_math)) {
- Opts.setFPExceptionMode(LangOptions::FPE_Ignore);
- }
----------------
As long as `-cc1` always contains `-ffp-exception-behavior` (which includes any
adjustment from `-ftrapping-math`), then it seems correct to drop this as this
patch is doing.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3080
}
Opts.setFPExceptionMode(FPEB);
----------------
SjoerdMeijer wrote:
> jansvoboda11 wrote:
> > SjoerdMeijer wrote:
> > > jansvoboda11 wrote:
> > > > The parsing of `OPT_ftrapping_math` and `OPT_fno_trapping_math` is
> > > > immediately overwritten here.
> > > Yeah, I did some work in this area some years ago, but that's a few years
> > > ago, and then in addition to that, we are talking about option handling
> > > in Clang which I always find very confusing... Just saying I can't
> > > remember too many details at this point.
> > >
> > > But starting with a first observation, you're saying that things are
> > > overwritten here, but they are different options. I.e., the deleted part
> > > honours `OPT_ftrapping_math`, and here exception modes are set based on
> > > `OPT_ffp_exception_behavior`. So, looks like there is some interaction
> > > here... Do we know how this should work?
> > I see. The thing is, before this patch, we call `Opts.setFPExceptionMode`
> > whenever we see either `OPT_ftrapping_math` or `OPT_fno_trapping_math`.
> >
> > But then, we **unconditionally** call `Opts.setFPExceptionMode(FPEB)` again
> > here. That **always** overwrites what we previously did for
> > `OPT_f[no_]trapping_math`, making that a dead code.
> >
> > `Opts.setFPExceptionMode()` doesn't do anything fancy, just assigns the
> > argument to the `Opts.FPExceptionModel` member.
> Ah yes, I actually missed that we always set `Opts.setFPExceptionMode(FPEB)`
> here! So that's clear now.
>
> But looks like my previous question is still relevant how these options
> should work together? I now see that
> `-ffp-exception-behavior` is a intel/microsoft special introduced in D62731.
>
> And while currently we ignore `-fftrapping-math` here, but that just seems
> like a bug, it looks like we actually need to handle it here too?
>
I just did an audit (see my other comments), and it looks like
`-ftrapping-math`'s behaviour is fully handled by the `-ffp-exception-mode`
option in `-cc1`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93395/new/
https://reviews.llvm.org/D93395
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
