[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+    if (FpPragmaCurrentLocation.isInvalid()) {
+      assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+             "Expected a default pragma float_control value");
----------------
mibintc wrote:
> yaxunl wrote:
> > mibintc wrote:
> > > yaxunl wrote:
> > > > This changes the behavior regarding AST reader and seems to be too hash restriction. Essentially this requires a pch can only be used with the same fp options with which the pch is generated. Since there are lots of fp options, it is impractical to generate pch for all the combinations.
> > > > 
> > > > We have seen regressions due to this assertion.
> > > > 
> > > > Can this assertion be dropped or done under some options?
> > > > 
> > > > Thanks.
> > > > 
> > > @yaxunl Can you please send me a reproducer, I'd like to see what's going on, not sure if just getting rid of the assertion will give the desired outcome. 
> > {F11915161}
> > 
> > Pls apply the patch.
> > 
> > Thanks.
> @rjmccall In the example supplied by @yaxunl, the floating point options in the pch file when created are default, and the floating point options in the use have no-signed-zeros flag.  The discrepancy causes an error diagnostic when the pch is used.  I added the FMF flags into FPFeatures in this patch, I made them COMPATIBLE_LANGOPT which is the encoding also being used for FastMath, FiniteMathOnly, and UnsafeFPMath.  Do you have some advice about this issue?  
A couple things are going on here.

First: a PCH can only end at the top level, not in the middle of a declaration, but otherwise Sema can be in an arbitrary semantic configuration.  That definitely includes arbitrary pragmas being in effect, so in general the end state might not match the default FP state, so this assertion is bogus.  When loading a PCH, you need to restore the pragma stack and current FP state to the configuration it was in at the end of the PCH.

Second: if you restore the pragma stack and FP state naively given the current representation of FP state, you will completely overwrite the FP settings of the current translation unit with the FP settings that were in effect when the PCH was built, which is obviously not okay.  This is one way (among several) that the current representation is not really living up to the statement that these language options are "compatible".  The better way to do this would be for the pragma stack and Expr nodes to record the current set of overrides in effect rather than the absolute current state; this could then be easily applied to an arbitrary global FP state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841