make “#pragma STDC FP_CONTRACT” on by default

[Abe]

Summary

Clang has the default FP contraction setting of “-ffp-contract=on”, which doesn`t really mean “on” in the conventional sense of the word, but rather really means “according to the per-statement effective value of the relevant pragma”.

Before this patch, Clang has that pragma defaulting to “off”. Since the “-ffp-contract=on” mode is really an AND of two booleans and the second of them defaults to “off”, the whole thing effectively defaults to “off”. This patch changes the default value of the pragma to “on”, thus making the default pair of booleans (on, on) rather than (on, off). This makes FP optimization slightly more aggressive than before when not using either “-Ofast”, “-ffast-math”, or “-ffp-contract=fast”. Even with this patch the compiler still respects “-ffp-contract=off”.

As per a suggestion by Steve Canon, the added code does _not_ require “-O3” or higher. This is so as to try our best to preserve identical floating-point results for unchanged source code compiling for an unchanged target when only changing from any optimization level in the set (“-O0”, “-O1”, “-O2”, “-O3”) to any other optimization level in that set. “-Os” and “-Oz” seem to be behaving identically, i.e. should probably be considered a part of the aforementioned set, but I have not reviewed this rigorously. “-Ofast” is explicitly _not_ a member of that set.

[scanon]

Abe, I had a patch to do the same thing last year (that we ended up backing out because it exposed a bug which has since been fixed). Your approach looks a bit different from the one that I took originally, and you seem to be missing some of the arm64 test changes that I had to put in:

https://reviews.llvm.org/D14200

Can you comment on the differences?

[Abe]

Added improved testing [both as improvements to existing tests and 1 entirely-new test] {copied from / in imitation of} https://reviews.llvm.org/D14200.

[Abe]

Fixed "fp_contract_7" of "clang/test/CodeGen/fp-contract-pragma.cpp" so it actually _does_ have multiple _not-provably-dead_ uses of the result of the multiplication [this nuance might be important in the future if a DCE pass will occur before the IR is tested].

[Abe]

Patch tweaking: renamed path prefixes from "clang/" to "cfe/trunk/", added _lots_ more context. Now the context shows up in Phabricator rather than "Context not available."

[Abe]

Removed some comments that I felt were good for clarity but at least 2 people disagreed.

[yaxunl]

Is it possible to merge cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O[0-3]___aarch64-backend.c as one and remove cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.h? They look the same except the RUN commands. Why not using one file with multiple RUn commands?

[Abe]

In D24481#547694, @yaxunl wrote:

Is it possible to merge cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O[0-3]___aarch64-backend.c as one and remove cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.h? They look the same except the RUN commands. Why not using one file with multiple RUn commands?

That seems like a good idea. I didn`t write it that way yet b/c I was unsure of the semantics of multiple "RUN" command-comments in a single file. If a single file has e.g.:

// RUN: %clang_cc1 -triple aarch64 -O1 -S -o - %s | FileCheck %s
// RUN: %clang_cc1 -triple aarch64 -O2 -S -o - %s | FileCheck %s
// RUN: %clang_cc1 -triple aarch64 -O3 -S -o - %s | FileCheck %s

... then that will cause 3 compilations and 3 tests, right?

[yaxunl]

... then that will cause 3 compilations and 3 tests, right?

Right.

[Abe]

Collapsed 4 test-case files that didn`t really need to be separate into 1 file, as suggested by Yaxun Liu.

[yaxunl]

clang/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.c

8 ↗

(On Diff #71959)

This check seems unnecessary and can be removed. The same as below. Then this test can be merged with fp-contract-pragma___on-by-default___-O0___aarch64-backend.c

[Abe]

Combined "fp-contract-pragma___on-by-default___-O0___aarch64-backend.c" and "fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.c" into a single file ["fp-contract-pragma___on-by-default.c"].

[yaxunl]

clang/lib/Frontend/CompilerInvocation.cpp

2441 ↗

(On Diff #72103)

The format of this line looks strange. Did you check with clang-format-diff.py if it is OK?

[Abe]

Minor edits for style-guidelines conformance.

[yaxunl]

LGTM. Thanks.

[scanon]

LGTM

[rengolin]

Folks, this commit has broken both AArch64 test-suite buildbots:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/3162

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/10449

I have reverted in r282289, let me know if you need help testing on AArch64.

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

ParsePreprocessorOutputArgs(Res.getPreprocessorOutputOpts(), Args,
                               Res.getFrontendOpts().ProgramAction);
 
   // Turn on -Wspir-compat for SPIR target.
   llvm::Triple T(Res.getTargetOpts().Triple);
   auto Arch = T.getArch();
   if (Arch == llvm::Triple::spir || Arch == llvm::Triple::spir64) {
     Res.getDiagnosticOpts().Warnings.push_back("spir-compat");
   }
+
+  if ((LangOpts.C11 || LangOpts.C99 || LangOpts.CPlusPlus) &&
+      (CodeGenOptions::FPC_On == Res.getCodeGenOpts().getFPContractMode()) &&
+      !LangOpts.CUDA)
+    LangOpts.DefaultFPContract = 1;
+
   return Success;
 }
 
 std::string CompilerInvocation::getModuleHash() const {
   // Note: For QoI reasons, the things we use as a hash here should all be
   // dumped via the -module-info flag.
   using llvm::hash_code;
   using llvm::hash_value;

cfe/trunk/test/CodeGen/aarch64-neon-fma.c

-// RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon -S -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon -ffp-contract=off -S -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
 
 // Test new aarch64 intrinsics and types
 
 #include <arm_neon.h>
 
 // CHECK-LABEL: define <2 x float> @test_vmla_n_f32(<2 x float> %a, <2 x float> %b, float %c) #0 {
 // CHECK:   [[VECINIT_I:%.*]] = insertelement <2 x float> undef, float %c, i32 0
 // CHECK:   [[VECINIT1_I:%.*]] = insertelement <2 x float> [[VECINIT_I]], float %c, i32 1

cfe/trunk/test/CodeGen/aarch64-scalar-fma.c

+// RUN: %clang_cc1 -triple=aarch64-unknown -Os -ffp-contract=fast -S -o - %s | FileCheck -check-prefix=CHECK-FAST -check-prefix=CHECK-ALL %s
+// RUN: %clang_cc1 -triple=aarch64-unknown -Os -ffp-contract=on -S -o - %s | FileCheck -check-prefix=CHECK-ON -check-prefix=CHECK-ALL %s
+// RUN: %clang_cc1 -triple=aarch64-unknown -Os -ffp-contract=off -S -o - %s | FileCheck -check-prefix=CHECK-OFF -check-prefix=CHECK-ALL %s
+// RUN: %clang_cc1 -triple=aarch64-unknown -Os -S -o - %s | FileCheck -check-prefix=CHECK-ON -check-prefix=CHECK-ALL %s
+// REQUIRES: aarch64-registered-target
+
+float test1(float x, float y, float z) {
+  return x*y + z;
+  // CHECK-ALL-LABEL: test1:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test2(double x, double y, double z) {
+  z -= x*y;
+  return z;
+  // CHECK-ALL-LABEL: test2:
+  // CHECK-FAST: fmsub
+  // CHECK-ON: fmsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+float test3(float x, float y, float z) {
+  float tmp = x*y;
+  return tmp + z;
+  // CHECK-ALL-LABEL: test3:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test4(double x, double y, double z) {
+  double tmp = x*y;
+  return tmp - z;
+  // CHECK-ALL-LABEL: test4:
+  // CHECK-FAST: fnmsub
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+#pragma STDC FP_CONTRACT ON
+
+float test5(float x, float y, float z) {
+  return x*y + z;
+  // CHECK-ALL-LABEL: test5:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test6(double x, double y, double z) {
+  z -= x*y;
+  return z;
+  // CHECK-ALL-LABEL: test6:
+  // CHECK-FAST: fmsub
+  // CHECK-ON: fmsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+float test7(float x, float y, float z) {
+  float tmp = x*y;
+  return tmp + z;
+  // CHECK-ALL-LABEL: test7:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test8(double x, double y, double z) {
+  double tmp = x*y;
+  return tmp - z;
+  // CHECK-ALL-LABEL: test8:
+  // CHECK-FAST: fnmsub
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+#pragma STDC FP_CONTRACT OFF
+
+float test9(float x, float y, float z) {
+  return x*y + z;
+  // CHECK-ALL-LABEL: test9:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test10(double x, double y, double z) {
+  z -= x*y;
+  return z;
+  // CHECK-ALL-LABEL: test10:
+  // CHECK-FAST: fmsub
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+float test11(float x, float y, float z) {
+  float tmp = x*y;
+  return tmp + z;
+  // CHECK-ALL-LABEL: test11:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test12(double x, double y, double z) {
+  double tmp = x*y;
+  return tmp - z;
+  // CHECK-ALL-LABEL: test12:
+  // CHECK-FAST: fnmsub
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+#pragma STDC FP_CONTRACT DEFAULT
+
+float test17(float x, float y, float z) {
+  return x*y + z;
+  // CHECK-ALL-LABEL: test17:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test18(double x, double y, double z) {
+  z -= x*y;
+  return z;
+  // CHECK-ALL-LABEL: test18:
+  // CHECK-FAST: fmsub
+  // CHECK-ON: fmsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}
+
+float test19(float x, float y, float z) {
+  float tmp = x*y;
+  return tmp + z;
+  // CHECK-ALL-LABEL: test19:
+  // CHECK-FAST: fmadd
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fadd
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fadd
+}
+
+double test20(double x, double y, double z) {
+  double tmp = x*y;
+  return tmp - z;
+  // CHECK-ALL-LABEL: test20:
+  // CHECK-FAST: fnmsub
+  // CHECK-ON: fmul
+  // CHECK-ON-NEXT: fsub
+  // CHECK-OFF: fmul
+  // CHECK-OFF-NEXT: fsub
+}

cfe/trunk/test/CodeGen/fp-contract-pragma.cpp

 // RUN: %clang_cc1 -O3 -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
 
-// Is FP_CONTRACT honored in a simple case?
-float fp_contract_1(float a, float b, float c) {
-// CHECK: _Z13fp_contract_1fff
+// Is FP_CONTRACT on by default, at least at -O3?
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
 // CHECK: tail call float @llvm.fmuladd
-  #pragma STDC FP_CONTRACT ON
   return a * b + c;
 }
 
 // Is FP_CONTRACT state cleared on exiting compound statements?
 float fp_contract_2(float a, float b, float c) {
 // CHECK: _Z13fp_contract_2fff
 // CHECK: %[[M:.+]] = fmul float %a, %b
 // CHECK-NEXT: fadd float %[[M]], %c
+  #pragma STDC FP_CONTRACT OFF
   {
     #pragma STDC FP_CONTRACT ON
   }
   return a * b + c;  
 }
 
 // Does FP_CONTRACT survive template instantiation?
-class Foo {};
-Foo operator+(Foo, Foo);
 
 template <typename T>
 T template_muladd(T a, T b, T c) {
   #pragma STDC FP_CONTRACT ON
   return a * b + c;
 }
 
 float fp_contract_3(float a, float b, float c) {

 #pragma STDC FP_CONTRACT OFF
 float fp_contract_6(float a, float b, float c) {
 // CHECK: _Z13fp_contract_6fff
 // CHECK: %[[M:.+]] = fmul float %a, %b
 // CHECK-NEXT: fadd float %[[M]], %c
   return a * b + c;
 }
 
+// Does FP_CONTRACT inside a function override the same in the file scope?
+float fp_contract_1(float a, float b, float c) {
+// CHECK: _Z13fp_contract_1fff
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b + c;
+}
+
+
 // If the multiply has multiple uses, don't produce fmuladd.
 // This used to assert (PR25719):
 // https://llvm.org/bugs/show_bug.cgi?id=25719
 
-float fp_contract_7(float a, float b, float c) {
+float fp_contract_7(float a, float b, float c, float& d_passed_by_ref) {
 // CHECK: _Z13fp_contract_7fff
 // CHECK:  %[[M:.+]] = fmul float %b, 2.000000e+00
-// CHECK-NEXT: fsub float %[[M]], %c
   #pragma STDC FP_CONTRACT ON
-  return (a = 2 * b) - c;
+  return (d_passed_by_ref = 2 * b) - c;
 }
 

cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default.c

+// RUN: %clang_cc1 -triple aarch64 -O0 -S -o - %s | FileCheck %s --check-prefix   ALL_BUILDS
+// RUN: %clang_cc1 -triple aarch64 -O1 -S -o - %s | FileCheck %s --check-prefixes ALL_BUILDS,NON_O0
+// RUN: %clang_cc1 -triple aarch64 -O2 -S -o - %s | FileCheck %s --check-prefixes ALL_BUILDS,NON_O0
+// RUN: %clang_cc1 -triple aarch64 -O3 -S -o - %s | FileCheck %s --check-prefixes ALL_BUILDS,NON_O0
+
+// REQUIRES: aarch64-registered-target
+
+// ALL_BUILDS-LABEL: fmadd_double:
+// ALL_BUILDS: fmadd d0, d{{[0-7]}}, d{{[0-7]}}, d{{[0-7]}}
+// NON_O0-NEXT: ret
+double fmadd_double(double a, double b, double c) {
+  return a*b+c;
+}
+
+// ALL_BUILDS: fmadd_single:
+// ALL_BUILDS: fmadd s0, s{{[0-7]}}, s{{[0-7]}}, s{{[0-7]}}
+// NON_O0-NEXT: ret
+float  fmadd_single(float  a, float  b, float  c) {
+  return a*b+c;
+}
+

cfe/trunk/test/Driver/clang_f_opts.c

 // RUN: %clang -### -x c++ -c -Wno-deprecated -Wdeprecated %s 2>&1 | FileCheck -check-prefix=DEPRECATED-ON-CHECK %s
 // RUN: %clang -### -x c++ -c -w %s 2>&1 | FileCheck -check-prefix=DEPRECATED-ON-CHECK %s
 // RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=DEPRECATED-OFF-CHECK %s
 // RUN: %clang -### -c -Wdeprecated %s 2>&1 | FileCheck -check-prefix=DEPRECATED-OFF-CHECK %s
 // DEPRECATED-ON-CHECK: -fdeprecated-macro
 // DEPRECATED-OFF-CHECK-NOT: -fdeprecated-macro
 
 // RUN: %clang -### -S -ffp-contract=fast %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-FAST-CHECK %s
-// RUN: %clang -### -S -ffast-math %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-FAST-CHECK %s
-// RUN: %clang -### -S -ffp-contract=off %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-OFF-CHECK %s
+// RUN: %clang -### -S -ffast-math        %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-FAST-CHECK %s
+// RUN: %clang -### -S -ffp-contract=off  %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-OFF-CHECK  %s
+// RUN: %clang -### -S -ffp-contract=on   %s 2>&1 | FileCheck -check-prefix=FP-CONTRACT-ON-CHECK   %s
+
 // FP-CONTRACT-FAST-CHECK: -ffp-contract=fast
-// FP-CONTRACT-OFF-CHECK: -ffp-contract=off
+// FP-CONTRACT-OFF-CHECK:  -ffp-contract=off
+// FP-CONTRACT-ON-CHECK:   -ffp-contract=on
 
 // RUN: %clang -### -S -funroll-loops %s 2>&1 | FileCheck -check-prefix=CHECK-UNROLL-LOOPS %s
 // RUN: %clang -### -S -fno-unroll-loops %s 2>&1 | FileCheck -check-prefix=CHECK-NO-UNROLL-LOOPS %s
 // RUN: %clang -### -S -fno-unroll-loops -funroll-loops %s 2>&1 | FileCheck -check-prefix=CHECK-UNROLL-LOOPS %s
 // RUN: %clang -### -S -funroll-loops -fno-unroll-loops %s 2>&1 | FileCheck -check-prefix=CHECK-NO-UNROLL-LOOPS %s
 // CHECK-UNROLL-LOOPS: "-funroll-loops"
 // CHECK-NO-UNROLL-LOOPS: "-fno-unroll-loops"