This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Remove -f[no-]trapping-math from -cc1 command line
ClosedPublic

Authored by jansvoboda11 on Dec 16 2020, 6:45 AM.

Details

Summary

This patch removes the -f[no-]trapping-math flags from the -cc1 command line. They are currently ignored in the command line parser anyways.

This does not remove -f[no-]trapping-math from the driver command line. The driver flags are being used and affect the compilation.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Dec 16 2020, 6:45 AM
jansvoboda11 requested review of this revision.Dec 16 2020, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 6:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 added inline comments.Dec 16 2020, 6:47 AM
clang/lib/Frontend/CompilerInvocation.cpp
2699

The parsing of OPT_ftrapping_math and OPT_fno_trapping_math is immediately overwritten here.

jansvoboda11 added a subscriber: SjoerdMeijer.

Tagging @SjoerdMeijer, as these flags were pulled into the -cc1 command-line in his patch: D23840.

This looks fine to be as that code is definitely dead. I would wait for @SjoerdMeijer in case there's a bug in the existing code.

jansvoboda11 added 1 blocking reviewer(s): SjoerdMeijer.Dec 21 2020, 2:09 AM
SjoerdMeijer added inline comments.Dec 21 2020, 2:29 AM
clang/lib/Frontend/CompilerInvocation.cpp
2699

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?

@SjoerdMeijer If you're not comfortable reviewing this, do you know of anyone who might want to take a look?

clang/lib/Frontend/CompilerInvocation.cpp
2699

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.

SjoerdMeijer added inline comments.Dec 21 2020, 3:02 AM
clang/lib/Frontend/CompilerInvocation.cpp
2699

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?

dexonsmith accepted this revision.Jan 6 2021, 1:16 PM

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.

clang/lib/Driver/ToolChains/Clang.cpp
2602–2605

FPExceptionBehavior and TrappingMath are set together here (in lock step).

2630–2651

Note this handling of -ftrapping-math in the driver, which uses it to set FPExceptionBehaviour correctly.

2702–2721

Note that -ffp-exception-behaviour also sets FPExceptionBehaviour (and TrappingMath).

2736–2737

This will override FPExceptionBehavior, (wiping out any effect of -ftrapping-math), but that seems intentional and TrappingMath is set the same way.

2743–2744

FPExceptionBehavior and TrappingMath are also set together here.

2833–2836

Note: given the above, this assertion seems sufficient for confirming -ftrapping-math is effectively passed to -cc1.

clang/lib/Frontend/CompilerInvocation.cpp
2687–2694

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.

2699

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.

Please also update the commit message to explain why this is safe (that -fp-exception-behavior fully encapsulates the semantics for -cc1) rather than just saying the options were ignored (which sounds like a bug).

SjoerdMeijer accepted this revision.Jan 7 2021, 7:35 AM

Thanks for getting to the bottom of this. Agreed, and also LGTM.

This revision is now accepted and ready to land.Jan 7 2021, 7:35 AM
This revision was landed with ongoing or failed builds.Jan 12 2021, 1:00 AM
This revision was automatically updated to reflect the committed changes.