Set FLT_EVAL_METHOD to -1 when fast-math is enabled.

[zahiraam]

Summary

Currently the control of the eval-method is mixed with fast-math. 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 may 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.

[andrew.w.kaylor]

The fix for the eval_method crash should be moved to a separate patch. Otherwise, this looks good. I have only minor comments.

clang/lib/Lex/PPMacroExpansion.cpp

1600

It looks like you've got tabs in the code here.

clang/lib/Sema/SemaAttr.cpp

528

Why don't you need the reverse of this in the Precise and Pop cases? Also, why is it necessary to call getCurrentFPEvalMethod() immediately after setting it?

clang/test/CodeGen/eval-method-fast-math.c

55 ↗

(On Diff #413479)

Can you add a test that uses the float_control push and pop?

[zahiraam]

clang/lib/Lex/PPMacroExpansion.cpp

1600

No. I think that just means that the block of code was shifted by a few spaces because it's in a condition now.

clang/lib/Sema/SemaAttr.cpp

528

The call to getCurrentFPEvalMethod() was just left over debugging. Removed it.

[aaron.ballman]

clang/lib/Sema/Sema.cpp

261

Shouldn't this be looking at getLangOpts().FastMath?

[zahiraam]

clang/lib/Sema/Sema.cpp

261

when the -ffast-math is enabled on the command line, it triggers all these math driver options:

"-menable-no-infs" "-menable-no-nans" "-fapprox-func" "-menable-unsafe-fp-math" "-fno-signed-zeros" "-mreassociate" "-freciprocal-math" "-ffp-contract=fast" "-ffast-math" "-ffinite-math-only"

That's a bit restrictive. We want the eval-method set to -1 when either reassoc or allowrecip are enabled.

[aaron.ballman]

clang/lib/Sema/Sema.cpp

261

Okay, then I think the comments about fast math should be fixed up; otherwise that's going to get confusing.

[aaron.ballman]

clang/test/CodeGen/eval-method-fast-math.c

83 ↗

(On Diff #414461)

What should happen in a case like this?

int whatever(float a, float b, float c) {
  #pragma float_control(precise, off)
  res = a + b + c;
  int val = __FLT_EVAL_METHOD__;
  #pragma float_control(precise, on)
  return __FLT_EVAL_METHOD__;
}

I would expect that int val would hold -1 and the return statement would return 0?

[zahiraam]

clang/test/CodeGen/eval-method-fast-math.c

83 ↗

(On Diff #414461)

This test case is failing with this error (at the second pragma):

t1.cpp:9:11: error: '#pragma float_control' can only appear at file scope or at
    the start of a compound statement
#pragma float_control(precise, off)
        ^
1 error generated.

Tried this:

int whatever(float a, float b, float c) {
#pragma float_control(precise, off)
res = a + b + c;
int val =__FLT_EVAL_METHOD__;
{
#pragma float_control(precise, on)
return __FLT_EVAL_METHOD__;
}

}

This generated these last IR instructions:

store float %add1, float* @"?res@@3MA", align 4
store i32 -1, i32* %val, align 4
ret i32 -1

Not sure if this is correct. I guess the first pragma (off) didn't popped, so it's still on? But inside the scope of the second pragma the first pragma shouldn't be visible? Not sure what should happen here.

[aaron.ballman]

clang/test/CodeGen/eval-method-fast-math.c

83 ↗

(On Diff #414461)

Not sure if this is correct.

It looks incorrect to me. What I'd expect is:

int whatever(float a, float b, float c) {
  #pragma float_control(precise, off)
  res = a + b + c;
  int val =__FLT_EVAL_METHOD__; // Expect -1
  {
    #pragma float_control(precise, on)
    return __FLT_EVAL_METHOD__; // Expect 0
  }
  return __FLT_EVAL_METHOD__; // Expect -1
}

I forgot about the function scope restrictions, but the file scope still has an interesting case worth testing:

float a = 1.0f, b = 2.0f, c = 3.0f;
#pragma float_control(precise, off)
float res = a + b + c;
int off =__FLT_EVAL_METHOD__; // Expect -1
#pragma float_control(precise, on)
float other_res = a + b + c;
int on = __FLT_EVAL_METHOD__; // Expect 0

Both of these are based on the idea that the float control pragma sets the flag until it's either popped (by leaving a compound statement) or it's changed to something else (by another pragma).

[aaron.ballman]

It looks like there are some clang-format issues to handle as well.

clang/test/CodeGen/eval-method-fast-math.c

113–116 ↗

(On Diff #414729)

Shouldn't there be CHECK lines for these?

[zahiraam]

clang/test/CodeGen/eval-method-fast-math.c

113–116 ↗

(On Diff #414729)

These are the ones:
// CHECK: store i32 0

// CHECK: ret i32 -1

they are truncated. They are in reality these:

store i32 -1, i32* %val1, align 4
store i32 0, i32* %val2, align 4

Is that what you mean?

[aaron.ballman]

Please fix the clang format issue as well. Other than that and a nit, this LGTM.

clang/lib/Lex/PPMacroExpansion.cpp

1587

Pretty sure 2 can be 1 so that we only create an array of one token, right?

[bjope]

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

[aaron.ballman]

In D121122#3402334, @bjope wrote:

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Sorry for the troubles!

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

The way I read the requirements in C2x 7.12p3 are that the types float_t and double_t are always defined in <math.h>, but in the event FLT_EVAL_METHOD isn't 0, 1, or 2, the implementation has to pick whatever type makes the most sense for the underlying types.

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

The standard doesn't admit that fast math is a thing; it's up to the implementations to define what their fast math extension does.

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

I think it should still be handled in <math.h>. *I am not a C floating point expert, so take this suggestion with a grain of salt*: I think it is defensible to fallback to defining float_t as float and double_t as double when the eval method is indeterminable. @andrew.w.kaylor may have more nuanced thoughts here.

[zahiraam]

In D121122#3402442, @aaron.ballman wrote:

In D121122#3402334, @bjope wrote:

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Sorry for the troubles!

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

The way I read the requirements in C2x 7.12p3 are that the types float_t and double_t are always defined in <math.h>, but in the event FLT_EVAL_METHOD isn't 0, 1, or 2, the implementation has to pick whatever type makes the most sense for the underlying types.

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

The standard doesn't admit that fast math is a thing; it's up to the implementations to define what their fast math extension does.

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

I think it should still be handled in <math.h>. *I am not a C floating point expert, so take this suggestion with a grain of salt*: I think it is defensible to fallback to defining float_t as float and double_t as double when the eval method is indeterminable. @andrew.w.kaylor may have more nuanced thoughts here.

Thanks for the answer @aaron.ballman.
Looking at this https://godbolt.org/z/drhGEjvns i see that float_t is defined in math.h (if I remove the include it will fail). Not sure what version of libs godbolt is using, but I tend to agree that it should be defined in math.h too. But Andy will have a more definite answer.

[bjope]

Looked around a bit.

• Seems like in GNU libc they use an internal __GLIBC_FLT_EVAL_METHOD that is set to 2 if __FLT_EVAL_METHOD__ is -1 (https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/flt-eval-method.h;hb=f60e45ba10f0ca2794318de95720cdbdb6ff20d0).
• In LLVM:s libc I do not see anything like that, instead there is an #error if __FLT_EVAL_METHOD__ is -1 (https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/float_t.h).
• Looking at newlib it seem to do nothing for __FLT_EVAL_METHOD__ equal to -1, although it has some comments about "Assume float_t and double_t have been defined previously for this configuration (e.g. config.h)" (https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/math.h;h=799ac494adc3f134a8f63a06397e833ec89c644a;hb=HEAD#l169)

So maybe I need to patch our newlib version somehow to make sure float_t is defined even for -1. But there could be problems with the LLVM libc implementation of math.h (which I think uses float_t.h).

clang/include/clang/Lex/Preprocessor.h

class Preprocessor {
   SourceLocation DATELoc, TIMELoc;
 
   // FEM_UnsetOnCommandLine means that an explicit evaluation method was
   // not specified on the command line. The target is queried to set the
   // default evaluation method.
   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.
   SourceLocation LastFPEvalPragmaLocation;
 
   LangOptions::FPEvalMethodKind TUFPEvalMethod =
       LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine;
 
public:
   LangOptions::FPEvalMethodKind getTUFPEvalMethod() const {
     return TUFPEvalMethod;
   }
 
   SourceLocation getLastFPEvalPragmaLocation() const {
     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 &&
            "FPEvalMethod should never be set to FEM_UnsetOnCommandLine");
     // This is the location of the '#pragma float_control" where the
     // execution state is modifed.
     LastFPEvalPragmaLocation = PragmaLoc;
     CurrentFPEvalMethod = Val;

clang/lib/Lex/PPMacroExpansion.cpp

if (II == Ident__LINE__) {
     } else {
       Result = "??? ??? ?? ??:??:?? ????\n";
     }
     // Surround the string with " and strip the trailing newline.
     OS << '"' << StringRef(Result).drop_back() << '"';
     Tok.setKind(tok::string_literal);
   } else if (II == Ident__FLT_EVAL_METHOD__) {
     // __FLT_EVAL_METHOD__ is set to the default value.
+    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.
+        // 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.
     OS << CounterValue++;
     Tok.setKind(tok::numeric_constant);
   } else if (II == Ident__has_feature) {
     EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this, false,
       [this](Token &Tok, bool &HasLexedNextToken) -> int {
         IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, *this,

clang/lib/Sema/Sema.cpp

if (getLangOpts().getFPEvalMethod() == LangOptions::FEM_UnsetOnCommandLine)
     // Use setting from TargetInfo.
     PP.setCurrentFPEvalMethod(SourceLocation(),
                               ctxt.getTargetInfo().getFPEvalMethod());
   else
     // Set initial value of __FLT_EVAL_METHOD__ from the command line.
     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.
 void Sema::anchor() {}
 
 void Sema::addImplicitTypedef(StringRef Name, QualType T) {
   DeclarationName DN = &Context.Idents.get(Name);
   if (IdResolver.begin(DN) == IdResolver.end())

clang/lib/Sema/SemaAttr.cpp

if ((Action == PSK_Push_Set || Action == PSK_Push || Action == PSK_Pop) &&
     return;
   }
   switch (Value) {
   default:
     llvm_unreachable("invalid pragma float_control kind");
   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)
       Diag(Loc, diag::err_pragma_fc_noprecise_requires_noexcept);
     else if (CurFPFeatures.getAllowFEnvAccess())
       Diag(Loc, diag::err_pragma_fc_noprecise_requires_nofenv);
     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())
       Diag(Loc, diag::err_pragma_fc_except_requires_precise);
     else
       NewFPFeatures.setFPExceptionModeOverride(LangOptions::FPE_Strict);
     FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures);
     break;
   case PFC_NoExcept:
     NewFPFeatures.setFPExceptionModeOverride(LangOptions::FPE_Ignore);
     FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures);
     break;
   case PFC_Push:
     FpPragmaStack.Act(Loc, Sema::PSK_Push_Set, StringRef(), NewFPFeatures);
     break;
   case PFC_Pop:
     if (FpPragmaStack.Stack.empty()) {
       Diag(Loc, diag::warn_pragma_pop_failed) << "float_control"
                                               << "stack empty";
       return;
     }
     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());
 }
 
 void Sema::ActOnPragmaMSPointersToMembers(
     LangOptions::PragmaMSPointersToMembersKind RepresentationMethod,
     SourceLocation PragmaLoc) {

clang/test/CodeGen/eval-method-fast-math.cpp

+// 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__;
+}