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

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:227
+  FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement() ||
+                       FPFeatures.allowFPContractWithinStatement());
 }
----------------
mibintc wrote:
> michele.scandale wrote:
> > I'm not convinced it correct to set `contract` when `allowFPContractWithinStatement` return true. Can someone clarify this?
> > 
> > If I compile the following example with `-ffp-contract=on`:
> > ```
> > float test1(float a, float b, float c) {
> >   float x = a * b;
> >   return x + c;
> > }
> > 
> > float test2(float a, float b, float c) {
> >   return a * b + c;
> > }
> > ```
> > 
> > Before this change the generated code was:
> > ```
> > define float @test1(float %a, float %b, float %c) {
> >   %0 = fmul float %a, %b
> >   %1 = fadd float %0, %c
> >   ret float %1
> > }
> > 
> > define float @test2(float %a, float %b, float %c) {
> >   %0 = call float @llvm.fmuladd.f32(float %a, float%b, float %c)
> >   ret float %0
> > }
> > ```
> > 
> > And my understanding is that the in-statement contraction is implemented by emitting the `llvm.fmuladd` call that a backend might decide to implement as `fmul + fadd` or as `fma`.
> > 
> > With this change the generated code is:
> > ```
> > define float @test1(float %a, float %b, float %c) {
> >   %0 = fmul contract float %a, %b
> >   %1 = fadd contract float %0, %c
> >   ret float %1
> > }
> > 
> > define float @test2(float %a, float %b, float %c) {
> >   %0 = call contract float @llvm.fmuladd.f32(float %a, float%b, float %c)
> >   ret float %0
> > }
> > ```
> > and it seems to me that in `test1` (where multiple statements where explicitly used) the optimizer is now allowed to perform the contraction, violating the original program semantic where only "in-statement" contraction was allowed.
> Thanks @michele.scandale i will work on a patch for this
@michele.scandale I posted a patch for 'contract' here, https://reviews.llvm.org/D79903 


================
Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+    if (FpPragmaCurrentLocation.isInvalid()) {
+      assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+             "Expected a default pragma float_control value");
----------------
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?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841