Currently the control of the eval-method is mixed with fast-math.

zahiraam · zahiraam · commit bbf0d1932a3c · 2022-03-17T11:48:03.000-07:00
FLT_EVAL_METHOD tells the user the precision at which, temporary results are evaluated but when fast-math is enabled, the numeric values are not guaranteed to match the source semantics, so the eval-method is meaningless. For example, the expression `x + y + z` has as source semantics `(x + y) + z`. FLT_EVAL_METHOD is telling the user at which precision `(x + y)` is evaluated. With fast-math enable the compiler can choose to evaluate the expression as `(y + z) + x`. The correct behavior is to set the FLT_EVAL_METHOD to `-1` to tell the user that the precision of the intermediate values is unknow. This patch is doing that. Differential Revision: https://reviews.llvm.org/D121122
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
@@ -192,6 +192,11 @@ class Preprocessor {
   LangOptions::FPEvalMethodKind CurrentFPEvalMethod =
       LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine;
 
+  // Keeps the value of the last evaluation method before a
+  // `pragma float_control (precise,off) is applied.
+  LangOptions::FPEvalMethodKind LastFPEvalMethod =
+      LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine;
+
   // The most recent pragma location where the floating point evaluation
   // method was modified. This is used to determine whether the
   // 'pragma clang fp eval_method' was used whithin the current scope.
@@ -2078,6 +2083,14 @@ class Preprocessor {
     return LastFPEvalPragmaLocation;
   }
 
+  LangOptions::FPEvalMethodKind getLastFPEvalMethod() const {
+    return LastFPEvalMethod;
+  }
+
+  void setLastFPEvalMethod(LangOptions::FPEvalMethodKind Val) {
+    LastFPEvalMethod = Val;
+  }
+
   void setCurrentFPEvalMethod(SourceLocation PragmaLoc,
                               LangOptions::FPEvalMethodKind Val) {
     assert(Val != LangOptions::FEM_UnsetOnCommandLine &&
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1577,14 +1577,35 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
     Tok.setKind(tok::string_literal);
   } else if (II == Ident__FLT_EVAL_METHOD__) {
     // __FLT_EVAL_METHOD__ is set to the default value.
-    OS << getTUFPEvalMethod();
-    // __FLT_EVAL_METHOD__ expands to a simple numeric value.
-    Tok.setKind(tok::numeric_constant);
-    if (getLastFPEvalPragmaLocation().isValid()) {
-      // The program is ill-formed. The value of __FLT_EVAL_METHOD__ is altered
-      // by the pragma.
-      Diag(Tok, diag::err_illegal_use_of_flt_eval_macro);
-      Diag(getLastFPEvalPragmaLocation(), diag::note_pragma_entered_here);
+    if (getTUFPEvalMethod() ==
+        LangOptions::FPEvalMethodKind::FEM_Indeterminable) {
+      // This is possible if `AllowFPReassoc` or `AllowReciprocal` is enabled.
+      // These modes can be triggered via the command line option `-ffast-math`
+      // or via a `pragam float_control`.
+      // __FLT_EVAL_METHOD__ expands to -1.
+      // The `minus` operator is the next token we read from the stream.
+      auto Toks = std::make_unique<Token[]>(1);
+      OS << "-";
+      Tok.setKind(tok::minus);
+      // Push the token `1` to the stream.
+      Token NumberToken;
+      NumberToken.startToken();
+      NumberToken.setKind(tok::numeric_constant);
+      NumberToken.setLiteralData("1");
+      NumberToken.setLength(1);
+      Toks[0] = NumberToken;
+      EnterTokenStream(std::move(Toks), 1, /*DisableMacroExpansion*/ false,
+                       /*IsReinject*/ false);
+    } else {
+      OS << getTUFPEvalMethod();
+      // __FLT_EVAL_METHOD__ expands to a simple numeric value.
+      Tok.setKind(tok::numeric_constant);
+      if (getLastFPEvalPragmaLocation().isValid()) {
+        // The program is ill-formed. The value of __FLT_EVAL_METHOD__ is
+        // altered by the pragma.
+        Diag(Tok, diag::err_illegal_use_of_flt_eval_macro);
+        Diag(getLastFPEvalPragmaLocation(), diag::note_pragma_entered_here);
+      }
     }
   } else if (II == Ident__COUNTER__) {
     // __COUNTER__ expands to a simple numeric value.
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
@@ -257,6 +257,12 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
     PP.setCurrentFPEvalMethod(SourceLocation(),
                               getLangOpts().getFPEvalMethod());
   CurFPFeatures.setFPEvalMethod(PP.getCurrentFPEvalMethod());
+  // When `-ffast-math` option is enabled, it triggers several driver math
+  // options to be enabled. Among those, only one the following two modes
+  // affect the eval-method:  reciprocal or reassociate.
+  if (getLangOpts().AllowFPReassoc || getLangOpts().AllowRecip)
+    PP.setCurrentFPEvalMethod(SourceLocation(),
+                              LangOptions::FEM_Indeterminable);
 }
 
 // Anchor Sema's type info to this TU.
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
@@ -508,6 +508,13 @@ void Sema::ActOnPragmaFloatControl(SourceLocation Loc,
   case PFC_Precise:
     NewFPFeatures.setFPPreciseEnabled(true);
     FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures);
+    if (PP.getCurrentFPEvalMethod() ==
+            LangOptions::FPEvalMethodKind::FEM_Indeterminable &&
+        PP.getLastFPEvalPragmaLocation().isValid())
+      // A preceding `pragma float_control(precise,off)` has changed
+      // the value of the evaluation method.
+      // Set it back to its old value.
+      PP.setCurrentFPEvalMethod(SourceLocation(), PP.getLastFPEvalMethod());
     break;
   case PFC_NoPrecise:
     if (CurFPFeatures.getFPExceptionMode() == LangOptions::FPE_Strict)
@@ -517,6 +524,10 @@ void Sema::ActOnPragmaFloatControl(SourceLocation Loc,
     else
       NewFPFeatures.setFPPreciseEnabled(false);
     FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures);
+    PP.setLastFPEvalMethod(PP.getCurrentFPEvalMethod());
+    // `AllowFPReassoc` or `AllowReciprocal` option is enabled.
+    PP.setCurrentFPEvalMethod(
+        Loc, LangOptions::FPEvalMethodKind::FEM_Indeterminable);
     break;
   case PFC_Except:
     if (!isPreciseFPEnabled())
@@ -540,6 +551,12 @@ void Sema::ActOnPragmaFloatControl(SourceLocation Loc,
     }
     FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures);
     NewFPFeatures = FpPragmaStack.CurrentValue;
+    if (CurFPFeatures.getAllowFPReassociate() ||
+        CurFPFeatures.getAllowReciprocal())
+      // Since we are popping the pragma, we don't want to be passing
+      // a location here.
+      PP.setCurrentFPEvalMethod(SourceLocation(),
+                                CurFPFeatures.getFPEvalMethod());
     break;
   }
   CurFPFeatures = NewFPFeatures.applyOverrides(getLangOpts());
diff --git a/clang/test/CodeGen/eval-method-fast-math.cpp b/clang/test/CodeGen/eval-method-fast-math.cpp
@@ -0,0 +1,117 @@
+// RUN: %clang_cc1 -fexperimental-strict-floating-point  \
+// RUN: -triple x86_64-linux-gnu -emit-llvm -o - %s  \
+// RUN: | FileCheck %s -check-prefixes=CHECK
+
+// RUN: %clang_cc1 -triple i386--linux -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefixes=CHECK-EXT
+
+// RUN: %clang_cc1 -fexperimental-strict-floating-point  \
+// RUN: -mreassociate -freciprocal-math -ffp-contract=fast \
+// RUN: -ffast-math -triple x86_64-linux-gnu \
+// RUN: -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefixes=CHECK-FAST
+
+// RUN: %clang_cc1 -triple i386--linux -mreassociate -freciprocal-math \
+// RUN: -ffp-contract=fast -ffast-math -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefixes=CHECK-FAST
+
+float a = 1.0f, b = 2.0f, c = 3.0f;
+#pragma float_control(precise, off)
+float res2 = a + b + c;
+int val3 = __FLT_EVAL_METHOD__;
+#pragma float_control(precise, on)
+float res3 = a + b + c;
+int val4 = __FLT_EVAL_METHOD__;
+
+// CHECK: @val3 = global i32 -1
+// CHECK: @val4 = global i32 0
+
+// CHECK-EXT: @val3 = global i32 -1
+// CHECK-EXT: @val4 = global i32 2
+
+// CHECK-FAST: @val3 = global i32 -1
+// CHECK-FAST: @val4 = global i32 -1
+
+float res;
+int add(float a, float b, float c) {
+  // CHECK: fadd float
+  // CHECK: load float, float*
+  // CHECK: fadd float
+  // CHECK: store float
+  // CHECK: ret i32 0
+  res = a + b + c;
+  return __FLT_EVAL_METHOD__;
+}
+
+int add_precise(float a, float b, float c) {
+#pragma float_control(precise, on)
+  // CHECK: fadd float
+  // CHECK: load float, float*
+  // CHECK: fadd float
+  // CHECK: store float
+  // CHECK: ret i32 0
+  res = a + b + c;
+  return __FLT_EVAL_METHOD__;
+}
+
+#pragma float_control(push)
+#pragma float_control(precise, on)
+int add_precise_1(float a, float b, float c) {
+  // CHECK: fadd float
+  // CHECK: load float, float*
+  // CHECK: fadd float
+  // CHECK: store float
+  // CHECK: ret i32 0
+  res = a + b + c;
+  return __FLT_EVAL_METHOD__;
+}
+#pragma float_control(pop)
+
+int add_not_precise(float a, float b, float c) {
+  // Fast-math is enabled with this pragma.
+#pragma float_control(precise, off)
+  // CHECK: fadd fast float
+  // CHECK: load float, float*
+  // CHECK: fadd fast float
+  // CHECK: float {{.*}}, float*
+  // CHECK: ret i32 -1
+  res = a + b + c;
+  return __FLT_EVAL_METHOD__;
+}
+
+#pragma float_control(push)
+// Fast-math is enabled with this pragma.
+#pragma float_control(precise, off)
+int add_not_precise_1(float a, float b, float c) {
+  // CHECK: fadd fast float
+  // CHECK: load float, float*
+  // CHECK: fadd fast float
+  // CHECK: float {{.*}}, float*
+  // CHECK: ret i32 -1
+  res = a + b + c;
+  return __FLT_EVAL_METHOD__;
+}
+#pragma float_control(pop)
+
+int getFPEvalMethod() {
+  // CHECK: ret i32 0
+  return __FLT_EVAL_METHOD__;
+}
+
+float res1;
+int whatever(float a, float b, float c) {
+#pragma float_control(precise, off)
+  // CHECK: load float, float*
+  // CHECK: fadd fast float
+  // CHECK: store float {{.*}}, float*
+  // CHECK: store i32 -1
+  // CHECK: store i32 0
+  // CHECK: ret i32 -1
+  res1 = a + b + c;
+  int val1 = __FLT_EVAL_METHOD__;
+  {
+#pragma float_control(precise, on)
+    int val2 = __FLT_EVAL_METHOD__;
+  }
+  return __FLT_EVAL_METHOD__;
+}
