[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2576
+    CmdArgs.push_back("-ftrapping-math");
+  } else if (TrappingMathPresent)
     CmdArgs.push_back("-fno-trapping-math");
----------------
mibintc wrote:
> michele.scandale wrote:
> > With this change if I run `clang -### -ffast-math test.c` I don't see `-fno-trapping-math` passed to the CC1. 
> > This is changing the changes the value of the function level attribute "no-trapping-math" (see lib/CodeGen/CGCall.cpp : 1747).
> > Is this an intended change?
> > 
> > Moreover since with this patch the default value for trapping math changed, the "no-trapping-math" function level attribute is incorrect also for default case.
> Before this patch, ftrapping-math was added to the Driver and also a bitfield, ``NoTrappingFPMath`` was created in the LLVM to describe the state of trapping-math, but otherwise that bit wasn't consulted and the option had no effect.  Gcc describes ftrapping-math as the default, but in llvm by default floating point exceptions are masked and this corresponds to the floating point Constrained Intrinsics having exception behavior set to ignored.  This patch changed the llvm constructor to set the trapping bit to "no trap".  In fact I'd like to get rid of the ``NoTrappingFPMath`` bitfield since it's not being used, but I didn't make that change at this point. 
> 
> If I remember correctly, there are a bunch of driver tests that failed if fno-trapping-math is output to cc1. I'd have to reconstruct the details.  Since fno-trapping-math is the default, it isn't passed through on the cc1 command line: the Clang.cpp driver doesn't pass through the positive and negative for each existing option.
> 
> Thanks for pointing out the line in CGCall.cpp, it seems the CodeGenOpts aren't getting set up perfectly I'll fix that in CompilerInvocation.cpp; I don't see anything setting trapping-math as part of function level attribute, @michele.scandale  did I overlook that/can you point out where that is?
I guess you are referring to the code in `TargetMachine.cpp` where the function level attributes are used to reset the `TargetOptions` state whenever we initiate the backend codegen for a given function. Considering that the trapping math option as stated in the documentation did not have any effect, I'm not surprised to see not many uses. The only one I can see is in `llvm/lib/Target/ARM/ARMAsmPrinter.cpp : 687` where the function level attribute affects the emission of some ARM specific attributes.

My only concern was that the change of the default value for trapping math was not propagated entirely causing this function level attribute to be initialized incorrectly.
Fixing the logic in `CompilerInvocation.cpp` considering the change of default it is fine for me.

Given that `ffp-exception-behavior={ignore,maytrap,strict}` supersedes `-f{,no-}trapping-math` I would expect long term to see the internal state of the compiler frontend to only care about the new state `FPExceptionBehavior` for both language and code generation options. And I guess the same would apply to the backend stage as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731