Add warning when eval-method is set in the presence of value unsafe floating-point calculations.

[zahiraam]

Summary

In fast-math mode, when unsafe math optimizations are enabled, the compiler is allowed to use optimizations that allow reassociation and transformations that don’t guaranty accuracy.
For example (x+y)+z is transformed into x+(y+z) . Although mathematically equivalent, these two expressions may not lead to the same final result due to errors of summation.
Or x/x is transformed into 1.0 but x could be 0.0, INF or NaN. And so this transformation also may not lead to the same final result.
Setting the eval method 'ffp-eval-method' or via '#pragma clang fp eval_method' in this mode, doesn’t have any effect.
This patch adds code to warn the user of this.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticCommonKinds.td

127

128–129

FWIW, I'm still not quite certain what the diagnostic is telling me. I don't know what "value-safe" means in this context, so we may need to tweak this further.

clang/lib/Parse/ParseStmt.cpp

1065

This looks like it's going to diagnose on every compound statement regardless of whether the function contains any floating-point operations at all. If so, that's way too chatty.

It seems like the issue here is "the command line options disagree", so why is this not a frontend warning when processing the command line options?

clang/test/Sema/eval-method-with-unsafe-math.c

37

Once we issue the warning for the pragma, we should not issue the warning a second time for the statement.

[zahiraam]

clang/lib/Parse/ParseStmt.cpp

1065

I tried to do that initially but I don't have a Location for the diagnostic.

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

1065

We issue plenty of diagnostics when command line options don't match expectations, as examples:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L525
https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L1241

(and so on).

[aaron.ballman]

clang/include/clang/Basic/DiagnosticCommonKinds.td

127–129

I think we should split the diagnostics into two -- one as a Sema diagnostic (as a warning for the pragma case, in the Pragmas diagnostic group) and one as a Frontend diagnostic (as an error, for the command line case).

Basically, I think we should treat the command line options as being mutually exclusive and handle it the same as any other "these options cannot be used together" situation.

clang/lib/Sema/Sema.cpp

259–260 ↗

(On Diff #417279)

I don't think this is the right place to diagnose this -- these are command line options that don't play well together, so they should be handled when processing command line options.

[zahiraam]

clang/lib/Sema/SemaAttr.cpp

492

Not sure if I should repeat the comment here (same one than in CompilerInvocation.cpp?).

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

Unless you have a strong reason for this to be a warning, this seems like a situation we should diagnose as an error with a much clearer message.

clang/include/clang/Basic/DiagnosticGroups.td

127 ↗

(On Diff #417356)

This change won't be needed any longer.

clang/include/clang/Basic/DiagnosticSemaKinds.td

6479–6481 ↗

(On Diff #417356)

Similar wording suggestion here as above -- this makes it more clear what the issue is and how the user can fix it. You'll have to figure out which pragmas and which command line options set the language options you're checking for the predicate. But the goal is to let users know where the conflict is.

Bonus points if you can add a note to point to the previous pragma that caused the conflict with clang fp eval_method so the user doesn't have to hunt for it. However, I don't insist on that as it may be difficult to track. But if it isn't difficult, it would allow us to improve the %select slightly to something like: "'#pragma clang fp eval_method' cannot be used with %select{'#pragma clang fp reassociate'|'-mreassociate'|'#pragma whatever'|'-fwhatever'|..}0">,

clang/lib/Sema/SemaAttr.cpp

492

Er, given that they're pretty involved comments, I'd only put the comments in one place, and then have the other one say "See the comments in <some reasonable identifying location> for more information about why this is diagnosed." or something to that effect.

[zahiraam]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

May be @andrew.w.kaylor would weigh in on this?

[andrew.w.kaylor]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

I was going to say that for the command line option we could just issue a warning saying that the later option overrides the earlier, but it's a bit complicated to sort out what that would mean if the eval method follows a fast-math option and it might not always be what the user intended. So, I guess I'd agree that it should be an error.

For the case with pragmas, the model I'd follow is the mixing of #pragma float_control(except, on) with a fast-math mode or #pragma float_control(precise, off) with a non-ignore exception mode. In both those cases we issue an error.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

For the case with pragmas, the model I'd follow is the mixing of #pragma float_control(except, on) with a fast-math mode or #pragma float_control(precise, off) with a non-ignore exception mode. In both those cases we issue an error.

Good catch, I think that's a good approach as well.

[zahiraam]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

I think i will have the issue with the order of appearance of the options on the command line.
RUN: -freciprocal-math -mreassociate -ffp-eval-method=source
and
RUN: -mreassociate -ffp-eval-method=source

will depend on which order I will test for LangOpts.ApproxFunc/AllowFPReasson/AllowRecip being used or not?

The run lines above might give the same diagnostic. Unless I do something really complicated to check the order of the options on the command line?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

I think i will have the issue with the order of appearance of the options on the command line.

You shouldn't -- you should be able to test the language options after the command line was fully parsed. See FixupInvocation() in CompilerInvocation.cpp.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53 ↗

(On Diff #417356)

I still prefer the suggested wording I had originally: "'-ffp-eval-method' cannot be used with '%0'"; I think it's a good generalization but still sufficiently informative. WDYT?

clang/include/clang/Basic/DiagnosticSemaKinds.td

6480 ↗

(On Diff #418210)

I'm not certain what pragma_clang_fp_eval_reassociate is?

6481–6489 ↗

(On Diff #418210)

These should be combined into one diagnostic, which I believe we wanted to be an error instead of a warning. Also, because this will trigger for pragma use OR command line argument use, I think we need to be more generalize about what it cannot be used with. e.g., '#pragma clang fp eval_method' cannot be used when %select{approximate functions|reassociation|reciprocal whatever}0 is enabled or something along those lines (I'm hoping @andrew.w.kaylor can help figure out what the best terminology is here, as I'm not super familiar with those floating-point features).

[aaron.ballman]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

52–53 ↗

(On Diff #418597)

clang/include/clang/Basic/DiagnosticSemaKinds.td

6479–6481 ↗

(On Diff #418597)

clang/test/Sema/eval-method-with-unsafe-math.c

53

Can you split the driver tests into their own file in the Driver directory?

[aaron.ballman]

LGTM, thanks!

clang/include/clang/Basic/DiagnosticCommonKinds.td

 def err_attribute_not_type_attr : Error<
   "%0 attribute cannot be applied to types">;
 def err_enum_template : Error<"enumeration cannot be a template">;
 
 def warn_cxx20_compat_consteval : Warning<
   "'consteval' specifier is incompatible with C++ standards before C++20">,
   InGroup<CXX20Compat>, DefaultIgnore;
 
+def warn_eval_method_setting_is_meaningless_in_value_unsafe_context: Warning<
+  "setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method`"
+  " is meaningless when numeric results of floating-point calculations aren't value-safe.">, InGroup<Pragmas>;
 }
 
 let CategoryName = "Nullability Issue" in {
 
 def warn_nullability_duplicate : Warning<
   "duplicate nullability specifier %0">,
   InGroup<Nullability>;
 

clang/lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
   Sema::CompoundScopeRAII CompoundScope(Actions, isStmtExpr);
 
   // Parse any pragmas at the beginning of the compound statement.
   ParseCompoundStatementLeadingPragmas();
   Actions.ActOnAfterCompoundStatementLeadingPragmas();
 
   StmtVector Stmts;
 
+  if (Tok.isNot(tok::annot_pragma_fp) &&
+      getLangOpts().getFPEvalMethod() != LangOptions::FEM_UnsetOnCommandLine &&
+      (getLangOpts().ApproxFunc || getLangOpts().AllowFPReassoc ||
+       getLangOpts().AllowRecip))
+    Diag(Tok.getLocation(),
+         diag::warn_eval_method_setting_is_meaningless_in_value_unsafe_context);
+
   // "__label__ X, Y, Z;" is the GNU "Local Label" extension.  These are
   // only allowed at the start of a compound stmt regardless of the language.
   while (Tok.is(tok::kw___label__)) {
     SourceLocation LabelLoc = ConsumeToken();
 
     SmallVector<Decl *, 8> DeclsInGroup;
     while (true) {
       if (Tok.isNot(tok::identifier)) {

clang/lib/Sema/SemaAttr.cpp

case LangOptions::FEM_Source:
     break;
   case LangOptions::FEM_Double:
     NewFPFeatures.setFPEvalMethodOverride(LangOptions::FEM_Double);
     break;
   case LangOptions::FEM_Extended:
     NewFPFeatures.setFPEvalMethodOverride(LangOptions::FEM_Extended);
     break;
   }
+  if (getLangOpts().ApproxFunc || getLangOpts().AllowFPReassoc ||
+      getLangOpts().AllowRecip)
+    Diag(Loc,
+         diag::warn_eval_method_setting_is_meaningless_in_value_unsafe_context);
   FpPragmaStack.Act(Loc, PSK_Set, StringRef(), NewFPFeatures);
   CurFPFeatures = NewFPFeatures.applyOverrides(getLangOpts());
   PP.setCurrentFPEvalMethod(Loc, Value);
 }
 
 void Sema::ActOnPragmaFloatControl(SourceLocation Loc,
                                    PragmaMsStackAction Action,
                                    PragmaFloatControlKind Value) {

clang/test/Sema/eval-method-with-unsafe-math.c

+// RUN: %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -ffp-eval-method=source \
+// RUN: -verify %s
+
+// RUN: %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -fapprox-func -ffp-eval-method=source \
+// RUN: -verify %s -DWARN
+
+// RUN: %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math  -ffp-eval-method=source \
+// RUN: -verify %s -DWARN
+
+// RUN: %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -mreassociate -ffp-eval-method=source \
+// RUN: -verify %s -DWARN
+
+#ifndef WARN
+// expected-no-diagnostics
+float f1(float a, float b, float c) {
+  return a * b + c;
+}
+
+float f2(float a, float b, float c) {
+#pragma clang fp eval_method(double)
+  return a * b + c;
+}
+
+#endif
+
+#ifdef WARN
+float f3(float a, float b, float c) {
+  // expected-warning@+1{{setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method` is meaningless when numeric results of floating-point calculations aren't value-safe.}}
+  return a * b + c;
+}
+
+float f4(float a, float b, float c) {
+  // expected-warning@+1{{setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method` is meaningless when numeric results of floating-point calculations aren't value-safe.}}
+#pragma clang fp eval_method(double)
+  // expected-warning@+1{{setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method` is meaningless when numeric results of floating-point calculations aren't value-safe.}}
+  return a * b + c;
+}
+#endif