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 ↗

(On Diff #416990)

128–129 ↗

(On Diff #416990)

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 ↗

(On Diff #416990)

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

38

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 ↗

(On Diff #416990)

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

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

1065 ↗

(On Diff #416990)

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 ↗

(On Diff #417279)

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

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

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

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

[andrew.w.kaylor]

clang/include/clang/Basic/DiagnosticFrontendKinds.td

50–53

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

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

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

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

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

I'm not certain what pragma_clang_fp_eval_reassociate is?

6481–6489

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

clang/include/clang/Basic/DiagnosticSemaKinds.td

6479–6481

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/DiagnosticFrontendKinds.td

def warn_fe_override_module : Warning<
     "overriding the module target triple with %0">,
     InGroup<DiagGroup<"override-module">>;
 def warn_fe_backend_unsupported_fp_rounding : Warning<
     "overriding currently unsupported rounding mode on this target">,
     InGroup<UnsupportedFPOpt>;
 def warn_fe_backend_unsupported_fp_exceptions : Warning<
     "overriding currently unsupported use of floating point exceptions "
     "on this target">, InGroup<UnsupportedFPOpt>;
 
+def err_incompatible_fp_eval_method_option_with_fapprox_func : Error<
+    "'fapprox-func' produces inaccurate arithmetic results. It is illegal when"
+    " 'ffp-eval-method' is set.">;
+def err_incompatible_fp_eval_method_option_with_mreassociate : Error<
+    "'mreassociate' produces inaccurate arithmetic results. It is illegal when"
+    " 'ffp-eval-method' is set.">;
+def err_incompatible_fp_eval_method_option_with_freciprocal : Error<
+    "'freciprocal' produces inaccurate arithmetic results. It is illegal when"
+    " 'ffp-eval-method' is set.">;
+
 def remark_fe_backend_optimization_remark : Remark<"%0">, BackendInfo,
     InGroup<BackendOptimizationRemark>;
 def remark_fe_backend_optimization_remark_missed : Remark<"%0">, BackendInfo,
     InGroup<BackendOptimizationRemarkMissed>;
 def remark_fe_backend_optimization_remark_analysis : Remark<"%0">, BackendInfo,
     InGroup<BackendOptimizationRemarkAnalysis>;
 def remark_fe_backend_optimization_remark_analysis_fpcommute : Remark<"%0; "
     "allow reordering by specifying '#pragma clang loop vectorize(enable)' "

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_pointer_sub_null_ptr : Warning<
   "performing pointer subtraction with a null pointer %select{has|may have}0 undefined behavior">,
   InGroup<NullPointerSubtraction>, DefaultIgnore;
 
 def warn_floatingpoint_eq : Warning<
   "comparing floating point with == or != is unsafe">,
   InGroup<DiagGroup<"float-equal">>, DefaultIgnore;
 
+def warn_pragma_clang_fp_reassociate_used_with_eval_method_setting : Warning<
+  "eval method setting via '%0' cannot be used with "
+  "'pragma_clang_fp_eval_reassociate'">, InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_fapprox_func : Warning<
+  "'#pragma clang fp eval_method' cannot be used with 'fapprox_func'">,
+  InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_mreassociate : Warning<
+  "'#pragma clang fp eval_method' cannot be used with 'mreassociate'">,
+  InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_freciprocal : Warning<
+  "'#pragma clang fp eval_method' cannot be used with 'freciprocal'">,
+  InGroup<Pragmas>;
+
 def warn_remainder_division_by_zero : Warning<
   "%select{remainder|division}0 by zero is undefined">,
   InGroup<DivZero>;
 def warn_shift_lhs_negative : Warning<"shifting a negative signed value is undefined">,
   InGroup<DiagGroup<"shift-negative-value">>;
 def warn_shift_negative : Warning<"shift count is negative">,
   InGroup<DiagGroup<"shift-count-negative">>;
 def warn_shift_gt_typewidth : Warning<"shift count >= width of type">,

clang/lib/Frontend/CompilerInvocation.cpp

static bool FixupInvocation(CompilerInvocation &Invocation,
   if (Args.hasArg(OPT_fgpu_allow_device_init) && !LangOpts.HIP)
     Diags.Report(diag::warn_ignored_hip_only_option)
         << Args.getLastArg(OPT_fgpu_allow_device_init)->getAsString(Args);
 
   if (Args.hasArg(OPT_gpu_max_threads_per_block_EQ) && !LangOpts.HIP)
     Diags.Report(diag::warn_ignored_hip_only_option)
         << Args.getLastArg(OPT_gpu_max_threads_per_block_EQ)->getAsString(Args);
 
+  // When these options are used, the compiler is allowed to apply
+  // optimizations that may affect the final result. For example
+  // (x+y)+z is transformed to x+(y+z) but may not give the same
+  // final result; it's not value safe.
+  // Another example can be to simplify x/x to 1.0 but x could be 0.0, INF
+  // or NaN. Final result may then differ. An error is issued when the eval
+  // method is set with one of these options.
+  if (Args.hasArg(OPT_ffp_eval_method_EQ)) {
+    if (LangOpts.ApproxFunc)
+      Diags.Report(
+          diag::err_incompatible_fp_eval_method_option_with_fapprox_func);
+    if (LangOpts.AllowFPReassoc)
+      Diags.Report(
+          diag::err_incompatible_fp_eval_method_option_with_mreassociate);
+    if (LangOpts.AllowRecip)
+      Diags.Report(
+          diag::err_incompatible_fp_eval_method_option_with_freciprocal);
+  }
+
   // -cl-strict-aliasing needs to emit diagnostic in the case where CL > 1.0.
   // This option should be deprecated for CL > 1.0 because
   // this option was added for compatibility with OpenCL 1.0.
   if (Args.getLastArg(OPT_cl_strict_aliasing) &&
       (LangOpts.getOpenCLCompatibleVersion() > 100))
     Diags.Report(diag::warn_option_invalid_ocl_version)
         << LangOpts.getOpenCLVersionString()
         << Args.getLastArg(OPT_cl_strict_aliasing)->getAsString(Args);

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)
+    Diag(Loc, diag::warn_pragma_clang_fp_eval_method_used_with_fapprox_func)
+        << "#pragma clang fp eval_method";
+  if (getLangOpts().AllowFPReassoc)
+    Diag(Loc, diag::warn_pragma_clang_fp_eval_method_used_with_mreassociate)
+        << "#pragma clang fp eval_method";
+  if (getLangOpts().AllowRecip)
+    Diag(Loc, diag::warn_pragma_clang_fp_eval_method_used_with_freciprocal)
+        << "#pragma clang fp eval_method";
   FpPragmaStack.Act(Loc, PSK_Set, StringRef(), NewFPFeatures);
   CurFPFeatures = NewFPFeatures.applyOverrides(getLangOpts());
   PP.setCurrentFPEvalMethod(Loc, Value);
 }
 
 void Sema::ActOnPragmaFloatControl(SourceLocation Loc,
                                    PragmaMsStackAction Action,
                                    PragmaFloatControlKind Value) {
void Sema::ActOnPragmaFPContract(SourceLocation Loc,
   case LangOptions::FPM_FastHonorPragmas:
     llvm_unreachable("Should not happen");
   }
   FpPragmaStack.Act(Loc, Sema::PSK_Set, StringRef(), NewFPFeatures);
   CurFPFeatures = NewFPFeatures.applyOverrides(getLangOpts());
 }
 
 void Sema::ActOnPragmaFPReassociate(SourceLocation Loc, bool IsEnabled) {
+  if (IsEnabled) {
+    StringRef Reason = "";
+    if (getLangOpts().getFPEvalMethod() != LangOptions::FEM_UnsetOnCommandLine)
+      Reason = "option ffp-eval-method";
+    // Here we could have both an option and a pragma that set the eval method.
+    // The pragma overrides the option in the command line.
+    if (PP.getLastFPEvalPragmaLocation().isValid())
+      Reason = "#pragma clang fp eval_method";
+    if (Reason != "")
+      Diag(Loc,
+           diag::warn_pragma_clang_fp_reassociate_used_with_eval_method_setting)
+          << Reason;
+  }
   FPOptionsOverride NewFPFeatures = CurFPFeatureOverrides();
   NewFPFeatures.setAllowFPReassociateOverride(IsEnabled);
   FpPragmaStack.Act(Loc, PSK_Set, StringRef(), NewFPFeatures);
   CurFPFeatures = NewFPFeatures.applyOverrides(getLangOpts());
 }
 
 void Sema::setRoundingMode(SourceLocation Loc, llvm::RoundingMode FPR) {
   // C2x: 7.6.2p3  If the FE_DYNAMIC mode is specified and FENV_ACCESS is "off",

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

+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu  \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu  -freciprocal-math \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=WARN-RECPR,WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu  -mreassociate \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=WARN-REC,WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu  -fapprox-func \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=WARN-FUNC,WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math -mreassociate -verify \
+// RUN: %s 2>&1 | FileCheck %s --check-prefixes=WARN-REC,WARN-RECPR,WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math -mreassociate -fapprox-func \
+// RUN: -verify %s 2>&1 \
+// RUN: | FileCheck %s --check-prefixes=WARN-FUNC,WARN-REC,WARN-RECPR,WARN-PRGM
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -fapprox-func -ffp-eval-method=source \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=CHECK-FUNC
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -mreassociate -ffp-eval-method=source \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=CHECK-ASSOC
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math -ffp-eval-method=source \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=CHECK-RECPR
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math -mreassociate \
+// RUN: -ffp-eval-method=source \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=CHECK-ASSOC,CHECK-RECPR
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -freciprocal-math -mreassociate -fapprox-func \
+// RUN: -ffp-eval-method=source -verify %s 2>&1 \
+// RUN: | FileCheck %s --check-prefixes=CHECK-ASSOC,CHECK-RECPR,CHECK-FUNC
+
+// RUN: not %clang_cc1 -fexperimental-strict-floating-point \
+// RUN: -triple x86_64-linux-gnu -ffp-eval-method=source \
+// RUN: -verify %s 2>&1 | FileCheck %s --check-prefixes=WARN-FUNC-OPT,WARN-PRGM
+
+// CHECK-FUNC: (frontend): 'fapprox-func' produces inaccurate arithmetic results. It is illegal when 'ffp-eval-method' is set.
+// CHECK-ASSOC: (frontend): 'mreassociate' produces inaccurate arithmetic results. It is illegal when 'ffp-eval-method' is set.
+// CHECK-RECPR: (frontend): 'freciprocal' produces inaccurate arithmetic results. It is illegal when 'ffp-eval-method' is set.
+
+// expected-no-diagnostics
+
+float f1(float a, float b, float c) {
+  a = b + c;
+  return a * b + c;
+}
+
+float f2(float a, float b, float c) {
+  // WARN-FUNC-OPT: eval method setting via 'option ffp-eval-method' cannot be used with 'pragma_clang_fp_eval_reassociate'
+#pragma clang fp reassociate (on)
+  return (a + b) + c;
+}
+
+float f3(float a, float b, float c) {
+#pragma clang fp reassociate (off)
+  return (a - b) - c;
+}
+
+float f4(float a, float b, float c) {
+#pragma clang fp eval_method (double)
+  // WARN-FUNC: '#pragma clang fp eval_method' cannot be used with 'fapprox_func'
+  // WARN-REC: '#pragma clang fp eval_method' cannot be used with 'mreassociate'
+  // WARN-RECPR: '#pragma clang fp eval_method' cannot be used with 'freciprocal'
+  // WARN-PRGM: eval method setting via '#pragma clang fp eval_method' cannot be used with 'pragma_clang_fp_eval_reassociate'
+#pragma clang fp reassociate (on)
+  return (a * c) - (b * c);
+}