Fix floating point math function attributes definition.

[michele.scandale]

Summary

With the support for changing the state of floating point optimizations
within a code block, we need to make sure that the function attributes
related to floating point math optimization are set in a conservative
way w.r.t. the body of the function to prevent illegal transformations
in the backends where such attributes are consumed.

[michele.scandale]

Rebase

[michele.scandale]

Rebase

[rjmccall]

I'll review this when it's rebased on top of the other commit.

[michele.scandale]

Rebase

[michele.scandale]

Rebase

[michele.scandale]

Rebase

[rjmccall]

Can we do this conservatively only if something is changed within the function definition, and otherwise respect the global settings?

[michele.scandale]

In D80462#2077569, @rjmccall wrote:

Can we do this conservatively only if something is changed within the function definition, and otherwise respect the global settings?

I'm not sure I understand what you mean. The initial state is already the global settings coming from the language options (see constructor of CodeGenFunction).
Then whenever we process a unary/binary floating point operator we consider the local FPOptions and adjust the values of the function attributes accordingly.

Maybe instead of having an explicit state (the booleans) in CodeGenFunction tracking the values for these function attributes, I could simply update the function attributes directly. I guess the extra cost is negligible.

[rjmccall]

We have a very similar problem with the constrained FP intrinsics where the use of the intrinsics anywhere in a function requires (1) an attribute to be set on the function and (2) all the other code in the function to be emitted with intrinsics. What I've been treating as the plan of record is that we'll handle this by setting an attribute on the (AST) function definition whenever we see a relevant local pragma within it. And if we're doing that anyway, I think we can use it for this as well: set them in getDefaultFunctionAttributes and then clear them off if we see the attribute.

As for the RAII objects, my biggest concern here is the amount of extra work every time we emit an FP expression when we should have already set up the default configuration correctly. In the recent refactor, we started only storing FPOptions when there are non-default options for an expression. We should be able to use that to stop retranslating FPOptions to LLVM's options on every expression. (We can consider optimizing it for consecutive operations under the same non-default settings later if it proves important.)

[michele.scandale]

Modify IR function attribute directly.
RAII object modifies IR builder state and function attributes only if needed.
Rebase.

[michele.scandale]

Store CGF in RAII object.

[rjmccall]

This looks great, thanks.

[michele.scandale]

Rebase

[michele.scandale]

As far as I can tell the failures reported shouldn't be caused by this change.

John, would you be able to land this on my behalf?

[rjmccall]

To ssh://github.com/llvm/llvm-project

a98d618f6e5f..7fac1acc6171  master -> master

clang/include/clang/Basic/LangOptions.h

private:
   /// Allow Reciprocal - Allow optimizations to use the reciprocal
   /// of an argument rather than perform division.
   unsigned allow_reciprocal : 1;
   /// Approximate functions - Allow substitution of approximate calculations
   /// for functions (sin, log, sqrt, etc).
   unsigned approx_func : 1;
 };
 
+inline bool operator==(FPOptions LHS, FPOptions RHS) {
+  return LHS.getAsOpaqueInt() == RHS.getAsOpaqueInt();
+}
+inline bool operator!=(FPOptions LHS, FPOptions RHS) {
+  return LHS.getAsOpaqueInt() != RHS.getAsOpaqueInt();
+}
+
 /// Describes the kind of translation unit being processed.
 enum TranslationUnitKind {
   /// The translation unit is a complete translation unit.
   TU_Complete,
 
   /// The translation unit is a prefix to a translation unit, and is
   /// not complete.
   TU_Prefix,
 
   /// The translation unit is a module.
   TU_Module
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_BASIC_LANGOPTIONS_H

clang/lib/CodeGen/CGExprScalar.cpp

static bool CanElideOverflowCheck(const ASTContext &Ctx, const BinOpInfo &Op) {
 
   // For unsigned multiplication the overflow check can be elided if either one
   // of the unpromoted types are less than half the size of the promoted type.
   unsigned PromotedSize = Ctx.getTypeSize(Op.E->getType());
   return (2 * Ctx.getTypeSize(LHSTy)) < PromotedSize ||
          (2 * Ctx.getTypeSize(RHSTy)) < PromotedSize;
 }
 
-static void setBuilderFlagsFromFPFeatures(CGBuilderTy &Builder,
-                                          CodeGenFunction &CGF,
-                                          FPOptions FPFeatures) {
-  auto NewRoundingBehavior = FPFeatures.getRoundingMode();
-  Builder.setDefaultConstrainedRounding(NewRoundingBehavior);
-  auto NewExceptionBehavior =
-      ToConstrainedExceptMD(FPFeatures.getExceptionMode());
-  Builder.setDefaultConstrainedExcept(NewExceptionBehavior);
-  CGF.SetFastMathFlags(FPFeatures);
-  assert((CGF.CurFuncDecl == nullptr || Builder.getIsFPConstrained() ||
-          isa<CXXConstructorDecl>(CGF.CurFuncDecl) ||
-          isa<CXXDestructorDecl>(CGF.CurFuncDecl) ||
-          (NewExceptionBehavior == llvm::fp::ebIgnore &&
-           NewRoundingBehavior == llvm::RoundingMode::NearestTiesToEven)) &&
-         "FPConstrained should be enabled on entire function");
-}
-
 class ScalarExprEmitter
   : public StmtVisitor<ScalarExprEmitter, Value*> {
   CodeGenFunction &CGF;
   CGBuilderTy &Builder;
   bool IgnoreResultAssign;
   llvm::LLVMContext &VMContext;
 public:
 
Value *EmitMul(const BinOpInfo &Ops) {
 
     if (Ops.Ty->isUnsignedIntegerType() &&
         CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
         !CanElideOverflowCheck(CGF.getContext(), Ops))
       return EmitOverflowCheckedBinOp(Ops);
 
     if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
       //  Preserve the old values
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF, Ops.FPFeatures);
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, Ops.FPFeatures);
       return Builder.CreateFMul(Ops.LHS, Ops.RHS, "mul");
     }
     if (Ops.isFixedPointOp())
       return EmitFixedPointBinOp(Ops);
     return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
   }
   /// Create a binary op that checks for overflow.
   /// Currently only supports +, - and *.
Value *ScalarExprEmitter::VisitUnaryLNot(const UnaryOperator *E) {
   // Perform vector logical not on comparison with zero vector.
   if (E->getType()->isVectorType() &&
       E->getType()->castAs<VectorType>()->getVectorKind() ==
           VectorType::GenericVector) {
     Value *Oper = Visit(E->getSubExpr());
     Value *Zero = llvm::Constant::getNullValue(Oper->getType());
     Value *Result;
     if (Oper->getType()->isFPOrFPVectorTy()) {
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF,
-                                    E->getFPFeatures(CGF.getLangOpts()));
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(
+          CGF, E->getFPFeatures(CGF.getLangOpts()));
       Result = Builder.CreateFCmp(llvm::CmpInst::FCMP_OEQ, Oper, Zero, "cmp");
     } else
       Result = Builder.CreateICmp(llvm::CmpInst::ICMP_EQ, Oper, Zero, "cmp");
     return Builder.CreateSExt(Result, ConvertType(E->getType()), "sext");
   }
 
   // Compare operand to zero.
   Value *BoolVal = CGF.EvaluateExprAsBool(E->getSubExpr());
if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
       llvm::Value *NonZero = Builder.CreateFCmpUNE(Ops.RHS, Zero);
       EmitBinOpCheck(std::make_pair(NonZero, SanitizerKind::FloatDivideByZero),
                      Ops);
     }
   }
 
   if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
     llvm::Value *Val;
-    llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-    setBuilderFlagsFromFPFeatures(Builder, CGF, Ops.FPFeatures);
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, Ops.FPFeatures);
     Val = Builder.CreateFDiv(Ops.LHS, Ops.RHS, "div");
     if (CGF.getLangOpts().OpenCL &&
         !CGF.CGM.getCodeGenOpts().CorrectlyRoundedDivSqrt) {
       // OpenCL v1.1 s7.4: minimum accuracy of single precision / is 2.5ulp
       // OpenCL v1.2 s5.6.4.2: The -cl-fp32-correctly-rounded-divide-sqrt
       // build option allows an application to specify that single precision
       // floating-point divide (x/y and 1/x) and sqrt used in the program
       // source are correctly rounded.
Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &op) {
   }
 
   if (op.Ty->isUnsignedIntegerType() &&
       CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
       !CanElideOverflowCheck(CGF.getContext(), op))
     return EmitOverflowCheckedBinOp(op);
 
   if (op.LHS->getType()->isFPOrFPVectorTy()) {
-    llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-    setBuilderFlagsFromFPFeatures(Builder, CGF, op.FPFeatures);
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, op.FPFeatures);
     // Try to form an fmuladd.
     if (Value *FMulAdd = tryEmitFMulAdd(op, CGF, Builder))
       return FMulAdd;
 
     return Builder.CreateFAdd(op.LHS, op.RHS, "add");
   }
 
   if (op.isFixedPointOp())
if (!op.LHS->getType()->isPointerTy()) {
     }
 
     if (op.Ty->isUnsignedIntegerType() &&
         CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
         !CanElideOverflowCheck(CGF.getContext(), op))
       return EmitOverflowCheckedBinOp(op);
 
     if (op.LHS->getType()->isFPOrFPVectorTy()) {
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF, op.FPFeatures);
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, op.FPFeatures);
       // Try to form an fmuladd.
       if (Value *FMulAdd = tryEmitFMulAdd(op, CGF, Builder, true))
         return FMulAdd;
       return Builder.CreateFSub(op.LHS, op.RHS, "sub");
     }
 
     if (op.isFixedPointOp())
       return EmitFixedPointBinOp(op);
if (LHSTy->isVectorType() && !E->getType()->isVectorType()) {
         Result = Builder.CreateTrunc(Result, Builder.getInt1Ty());
       return EmitScalarConversion(Result, CGF.getContext().BoolTy, E->getType(),
                                   E->getExprLoc());
     }
 
     if (BOInfo.isFixedPointOp()) {
       Result = EmitFixedPointBinOp(BOInfo);
     } else if (LHS->getType()->isFPOrFPVectorTy()) {
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF, BOInfo.FPFeatures);
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, BOInfo.FPFeatures);
       if (!IsSignaling)
         Result = Builder.CreateFCmp(FCmpOpc, LHS, RHS, "cmp");
       else
         Result = Builder.CreateFCmpS(FCmpOpc, LHS, RHS, "cmp");
     } else if (LHSTy->hasSignedIntegerRepresentation()) {
       Result = Builder.CreateICmp(SICmpOpc, LHS, RHS, "cmp");
     } else {
       // Unsigned integers and pointers.
Value *ScalarExprEmitter::VisitBinLAnd(const BinaryOperator *E) {
   // Perform vector logical and on comparisons with zero vectors.
   if (E->getType()->isVectorType()) {
     CGF.incrementProfileCounter(E);
 
     Value *LHS = Visit(E->getLHS());
     Value *RHS = Visit(E->getRHS());
     Value *Zero = llvm::ConstantAggregateZero::get(LHS->getType());
     if (LHS->getType()->isFPOrFPVectorTy()) {
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF,
-                                    E->getFPFeatures(CGF.getLangOpts()));
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(
+          CGF, E->getFPFeatures(CGF.getLangOpts()));
       LHS = Builder.CreateFCmp(llvm::CmpInst::FCMP_UNE, LHS, Zero, "cmp");
       RHS = Builder.CreateFCmp(llvm::CmpInst::FCMP_UNE, RHS, Zero, "cmp");
     } else {
       LHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, LHS, Zero, "cmp");
       RHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, RHS, Zero, "cmp");
     }
     Value *And = Builder.CreateAnd(LHS, RHS);
     return Builder.CreateSExt(And, ConvertType(E->getType()), "sext");
Value *ScalarExprEmitter::VisitBinLOr(const BinaryOperator *E) {
   // Perform vector logical or on comparisons with zero vectors.
   if (E->getType()->isVectorType()) {
     CGF.incrementProfileCounter(E);
 
     Value *LHS = Visit(E->getLHS());
     Value *RHS = Visit(E->getRHS());
     Value *Zero = llvm::ConstantAggregateZero::get(LHS->getType());
     if (LHS->getType()->isFPOrFPVectorTy()) {
-      llvm::IRBuilder<>::FastMathFlagGuard FMFG(Builder);
-      setBuilderFlagsFromFPFeatures(Builder, CGF,
-                                    E->getFPFeatures(CGF.getLangOpts()));
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(
+          CGF, E->getFPFeatures(CGF.getLangOpts()));
       LHS = Builder.CreateFCmp(llvm::CmpInst::FCMP_UNE, LHS, Zero, "cmp");
       RHS = Builder.CreateFCmp(llvm::CmpInst::FCMP_UNE, RHS, Zero, "cmp");
     } else {
       LHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, LHS, Zero, "cmp");
       RHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, RHS, Zero, "cmp");
     }
     Value *Or = Builder.CreateOr(LHS, RHS);
     return Builder.CreateSExt(Or, ConvertType(E->getType()), "sext");

clang/lib/CodeGen/CodeGenFunction.h

public:
     }
   };
 
   template <class T>
   typename DominatingValue<T>::saved_type saveValueInCond(T value) {
     return DominatingValue<T>::save(*this, value);
   }
 
+  class CGFPOptionsRAII {
+  public:
+    CGFPOptionsRAII(CodeGenFunction &CGF, FPOptions FPFeatures);
+    ~CGFPOptionsRAII();
+
+  private:
+    CodeGenFunction &CGF;
+    FPOptions OldFPFeatures;
+    Optional<CGBuilderTy::FastMathFlagGuard> FMFGuard;
+  };
+  FPOptions CurFPFeatures;
+
 public:
   /// ObjCEHValueStack - Stack of Objective-C exception values, used for
   /// rethrows.
   SmallVector<llvm::Value*, 8> ObjCEHValueStack;
 
   /// A class controlling the emission of a finally block.
   class FinallyInfo {
     /// Where the catchall's edge through the cleanup should go.

clang/lib/CodeGen/CodeGenFunction.cpp

static bool shouldEmitLifetimeMarkers(const CodeGenOptions &CGOpts,
   // For now, only in optimized builds.
   return CGOpts.OptimizationLevel != 0;
 }
 
 CodeGenFunction::CodeGenFunction(CodeGenModule &cgm, bool suppressNewContext)
     : CodeGenTypeCache(cgm), CGM(cgm), Target(cgm.getTarget()),
       Builder(cgm, cgm.getModule().getContext(), llvm::ConstantFolder(),
               CGBuilderInserterTy(this)),
-      SanOpts(CGM.getLangOpts().Sanitize), DebugInfo(CGM.getModuleDebugInfo()),
-      PGO(cgm), ShouldEmitLifetimeMarkers(shouldEmitLifetimeMarkers(
-                    CGM.getCodeGenOpts(), CGM.getLangOpts())) {
+      SanOpts(CGM.getLangOpts().Sanitize), CurFPFeatures(CGM.getLangOpts()),
+      DebugInfo(CGM.getModuleDebugInfo()), PGO(cgm),
+      ShouldEmitLifetimeMarkers(
+          shouldEmitLifetimeMarkers(CGM.getCodeGenOpts(), CGM.getLangOpts())) {
   if (!suppressNewContext)
     CGM.getCXXABI().getMangleContext().startNewFunction();
 
-  SetFastMathFlags(FPOptions(CGM.getLangOpts()));
+  SetFastMathFlags(CurFPFeatures);
   SetFPModel();
 }
 
 CodeGenFunction::~CodeGenFunction() {
   assert(LifetimeExtendedCleanupStack.empty() && "failed to emit a cleanup");
 
   // If there are any unclaimed block infos, go ahead and destroy them
   // now.  This can happen if IR-gen gets clever and skips evaluating
void CodeGenFunction::SetFastMathFlags(FPOptions FPFeatures) {
   FMF.setNoInfs(FPFeatures.noHonorInfs());
   FMF.setNoSignedZeros(FPFeatures.noSignedZeros());
   FMF.setAllowReciprocal(FPFeatures.allowReciprocalMath());
   FMF.setApproxFunc(FPFeatures.allowApproximateFunctions());
   FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement());
   Builder.setFastMathFlags(FMF);
 }
 
+CodeGenFunction::CGFPOptionsRAII::CGFPOptionsRAII(CodeGenFunction &CGF,
+                                                  FPOptions FPFeatures)
+    : CGF(CGF), OldFPFeatures(CGF.CurFPFeatures) {
+  CGF.CurFPFeatures = FPFeatures;
+
+  if (OldFPFeatures == FPFeatures)
+    return;
+
+  FMFGuard.emplace(CGF.Builder);
+
+  auto NewRoundingBehavior = FPFeatures.getRoundingMode();
+  CGF.Builder.setDefaultConstrainedRounding(NewRoundingBehavior);
+  auto NewExceptionBehavior =
+      ToConstrainedExceptMD(FPFeatures.getExceptionMode());
+  CGF.Builder.setDefaultConstrainedExcept(NewExceptionBehavior);
+
+  CGF.SetFastMathFlags(FPFeatures);
+
+  assert((CGF.CurFuncDecl == nullptr || CGF.Builder.getIsFPConstrained() ||
+          isa<CXXConstructorDecl>(CGF.CurFuncDecl) ||
+          isa<CXXDestructorDecl>(CGF.CurFuncDecl) ||
+          (NewExceptionBehavior == llvm::fp::ebIgnore &&
+           NewRoundingBehavior == llvm::RoundingMode::NearestTiesToEven)) &&
+         "FPConstrained should be enabled on entire function");
+
+  auto mergeFnAttrValue = [&](StringRef Name, bool Value) {
+    auto OldValue =
+        CGF.CurFn->getFnAttribute(Name).getValueAsString() == "true";
+    auto NewValue = OldValue & Value;
+    if (OldValue != NewValue)
+      CGF.CurFn->addFnAttr(Name, llvm::toStringRef(NewValue));
+  };
+  mergeFnAttrValue("no-infs-fp-math", FPFeatures.noHonorInfs());
+  mergeFnAttrValue("no-nans-fp-math", FPFeatures.noHonorNaNs());
+  mergeFnAttrValue("no-signed-zeros-fp-math", FPFeatures.noSignedZeros());
+  mergeFnAttrValue(
+      "unsafe-fp-math",
+      FPFeatures.allowAssociativeMath() && FPFeatures.allowReciprocalMath() &&
+          FPFeatures.allowApproximateFunctions() && FPFeatures.noSignedZeros());
+}
+
+CodeGenFunction::CGFPOptionsRAII::~CGFPOptionsRAII() {
+  CGF.CurFPFeatures = OldFPFeatures;
+}
+
 LValue CodeGenFunction::MakeNaturalAlignAddrLValue(llvm::Value *V, QualType T) {
   LValueBaseInfo BaseInfo;
   TBAAAccessInfo TBAAInfo;
   CharUnits Alignment = CGM.getNaturalTypeAlignment(T, &BaseInfo, &TBAAInfo);
   return LValue::MakeAddr(Address(V, Alignment), T, getContext(), BaseInfo,
                           TBAAInfo);
 }
 

clang/test/CodeGen/fp-function-attrs.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffp-contract=fast -emit-llvm -o - %s | FileCheck %s
+
+float test_default(float a, float b, float c) {
+  float tmp = a;
+  tmp += b;
+  tmp += c;
+  return tmp;
+}
+
+// CHECK: define float @_Z12test_defaultfff(float %a, float %b, float %c) [[FAST_ATTRS:#[0-9]+]]
+// CHECK: fadd fast float {{%.+}}, {{%.+}}
+// CHECK: fadd fast float {{%.+}}, {{%.+}}
+
+float test_precise_on_pragma(float a, float b, float c) {
+  float tmp = a;
+  {
+    #pragma float_control(precise, on)
+    tmp += b;
+  }
+  tmp += c;
+  return tmp;
+}
+
+// CHECK: define float @_Z22test_precise_on_pragmafff(float %a, float %b, float %c) [[PRECISE_ATTRS:#[0-9]+]]
+// CHECK: fadd float {{%.+}}, {{%.+}}
+// CHECK: fadd fast float {{%.+}}, {{%.+}}
+
+float test_reassociate_off_pragma(float a, float b, float c) {
+  float tmp = a;
+  {
+    #pragma clang fp reassociate(off)
+    tmp += b;
+  }
+  tmp += c;
+  return tmp;
+}
+
+// CHECK: define float @_Z27test_reassociate_off_pragmafff(float %a, float %b, float %c) [[NOREASSOC_ATTRS:#[0-9]+]]
+// CHECK: fadd nnan ninf nsz arcp contract afn float {{%.+}}, {{%.+}}
+// CHECK: fadd fast float {{%.+}}, {{%.+}}
+
+// CHECK: attributes [[FAST_ATTRS]] = { {{.*}}"no-infs-fp-math"="true" {{.*}}"no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" {{.*}}"unsafe-fp-math"="true"{{.*}} }
+// CHECK: attributes [[PRECISE_ATTRS]] = { {{.*}}"no-infs-fp-math"="false" {{.*}}"no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" {{.*}}"unsafe-fp-math"="false"{{.*}} }
+// CHECK: attributes [[NOREASSOC_ATTRS]] = { {{.*}}"no-infs-fp-math"="true" {{.*}}"no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" {{.*}}"unsafe-fp-math"="false"{{.*}} }