[TM] Restore default TargetOptions in TargetMachine::resetTargetOptions.

[jlebar]

Summary

Previously if you had

• a function with the fast-math-enabled attr, followed by
• a function without the fast-math attr,

the second function would inherit the first function's fast-math-ness.

This means that mixing fast-math and non-fast-math functions in a module
was completely broken unless you explicitly annotated every
non-fast-math function with "unsafe-fp-math"="false". This appears to
have been broken since at r176986 (March 2013), when the
resetTargetOptions function was introduced.

This patch tests the correct behavior as best we can. I don't think I
can test FPDenormalMode and NoTrappingFPMath, because they aren't used
in any backends during function lowering. Surprisingly, I also can't
find any uses at all of LessPreciseFPMAD affecting generated code.

The NVPTX/fast-math.ll test changes are an expected result of fixing
this bug. When FMA is disabled, we emit add as "add.rn.f32", which
prevents fma combining. Before this patch, fast-math was enabled in all
functions following the one which explicitly enabled it on itself, so we
were emitting plain "add.f32" where we should have generated
"add.rn.f32".

[jlebar]

Shout out to rr [1]. I went from "huh, this seems wrong" to finding the responsible code in about 30 seconds. Put a breakpoint where TargetOptions.UnsafeFPMath is read, hit the breakpoint, watch -l TargetOptions.UnsafeFPMath, reverse continue. Bam.

[1] http://rr-project.org/

[majnemer]

LGTM

[echristo]

LGTM, thanks. I was looking for a reproducer for this for a while :)

[jlebar]

Thank you for the reviews, everyone!

[jlebar]

This comment was removed by jlebar.

llvm/trunk/include/llvm/Target/TargetMachine.h

protected: // Can only create subclasses.
   const MCRegisterInfo *MRI;
   const MCInstrInfo *MII;
   const MCSubtargetInfo *STI;
 
   unsigned RequireStructuredCFG : 1;
   unsigned O0WantsFastISel : 1;
 
 public:
+  const TargetOptions DefaultOptions;
   mutable TargetOptions Options;
 
   virtual ~TargetMachine();
 
   const Target &getTarget() const { return TheTarget; }
 
   const Triple &getTargetTriple() const { return TargetTriple; }
   StringRef getTargetCPU() const { return TargetCPU; }

llvm/trunk/lib/Target/TargetMachine.cpp

 // TargetMachine Class
 //
 
 TargetMachine::TargetMachine(const Target &T, StringRef DataLayoutString,
                              const Triple &TT, StringRef CPU, StringRef FS,
                              const TargetOptions &Options)
     : TheTarget(T), DL(DataLayoutString), TargetTriple(TT), TargetCPU(CPU),
       TargetFS(FS), AsmInfo(nullptr), MRI(nullptr), MII(nullptr), STI(nullptr),
-      RequireStructuredCFG(false), Options(Options) {
+      RequireStructuredCFG(false), DefaultOptions(Options), Options(Options) {
   if (EnableIPRA.getNumOccurrences())
     this->Options.EnableIPRA = EnableIPRA;
 }
 
 TargetMachine::~TargetMachine() {
   delete AsmInfo;
   delete MRI;
   delete MII;
   delete STI;
 }
 
 bool TargetMachine::isPositionIndependent() const {
   return getRelocationModel() == Reloc::PIC_;
 }
 
 /// \brief Reset the target options based on the function's attributes.
 // FIXME: This function needs to go away for a number of reasons:
 // a) global state on the TargetMachine is terrible in general,
-// b) there's no default state here to keep,
-// c) these target options should be passed only on the function
+// b) these target options should be passed only on the function
 //    and not on the TargetMachine (via TargetOptions) at all.
 void TargetMachine::resetTargetOptions(const Function &F) const {
 #define RESET_OPTION(X, Y)                                                     \
   do {                                                                         \
     if (F.hasFnAttribute(Y))                                                   \
       Options.X = (F.getFnAttribute(Y).getValueAsString() == "true");          \
+    else                                                                       \
+      Options.X = DefaultOptions.X;                                            \
   } while (0)
 
   RESET_OPTION(LessPreciseFPMADOption, "less-precise-fpmad");
   RESET_OPTION(UnsafeFPMath, "unsafe-fp-math");
   RESET_OPTION(NoInfsFPMath, "no-infs-fp-math");
   RESET_OPTION(NoNaNsFPMath, "no-nans-fp-math");
   RESET_OPTION(NoTrappingFPMath, "no-trapping-math");
 
   StringRef Denormal =
     F.getFnAttribute("denormal-fp-math").getValueAsString();
   if (Denormal == "ieee")
     Options.FPDenormalMode = FPDenormal::IEEE;
   else if (Denormal == "preserve-sign")
     Options.FPDenormalMode = FPDenormal::PreserveSign;
   else if (Denormal == "positive-zero")
     Options.FPDenormalMode = FPDenormal::PositiveZero;
+  else
+    Options.FPDenormalMode = DefaultOptions.FPDenormalMode;
 }
 
 /// Returns the code generation relocation model. The choices are static, PIC,
 /// and dynamic-no-pic.
 Reloc::Model TargetMachine::getRelocationModel() const { return RM; }
 
 /// Returns the code model. The choices are small, kernel, medium, large, and
 /// target default.

llvm/trunk/test/CodeGen/NVPTX/fast-math.ll

 ; CHECK: div.approx.f32
 define float @sqrt_div_fast(float %a, float %b) #0 {
   %t1 = tail call float @llvm.nvvm.sqrt.f(float %a)
   %t2 = fdiv float %t1, %b
   ret float %t2
 }
 
 ; CHECK-LABEL: fadd
-; CHECK: add.f32
+; CHECK: add.rn.f32
 define float @fadd(float %a, float %b) {
   %t1 = fadd float %a, %b
   ret float %t1
 }
 
 ; CHECK-LABEL: fadd_ftz
-; CHECK: add.ftz.f32
+; CHECK: add.rn.ftz.f32
 define float @fadd_ftz(float %a, float %b) #1 {
   %t1 = fadd float %a, %b
   ret float %t1
 }
 
 attributes #0 = { "unsafe-fp-math" = "true" }
 attributes #1 = { "nvptx-f32ftz" = "true" }

llvm/trunk/test/CodeGen/PowerPC/change-no-infs.ll

+; Check that we can enable/disable NoInfsFPMath and NoNaNsInFPMath via function
+; attributes.  An attribute on one function should not magically apply to the
+; next one.
+
+; RUN: llc < %s -mtriple=powerpc64-unknown-unknown -mcpu=pwr7 -mattr=-vsx \
+; RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=SAFE
+
+; RUN: llc < %s -mtriple=powerpc64-unknown-unknown -mcpu=pwr7 -mattr=-vsx \
+; RUN:   -enable-no-infs-fp-math -enable-no-nans-fp-math \
+; RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=UNSAFE
+
+; The fcmp+select in these functions should be converted to a fsel instruction
+; when both NoInfsFPMath and NoNaNsInFPMath are enabled.
+
+; CHECK-LABEL: default0:
+define double @default0(double %a, double %y, double %z) {
+entry:
+; SAFE-NOT:  fsel
+; UNSAFE:    fsel
+  %cmp = fcmp ult double %a, 0.000000e+00
+  %z.y = select i1 %cmp, double %z, double %y
+  ret double %z.y
+}
+
+; CHECK-LABEL: unsafe_math_off:
+define double @unsafe_math_off(double %a, double %y, double %z) #0 #2 {
+entry:
+; SAFE-NOT:   fsel
+; UNSAFE-NOT: fsel
+  %cmp = fcmp ult double %a, 0.000000e+00
+  %z.y = select i1 %cmp, double %z, double %y
+  ret double %z.y
+}
+
+; CHECK-LABEL: default1:
+define double @default1(double %a, double %y, double %z) {
+; SAFE-NOT:  fsel
+; UNSAFE:    fsel
+  %cmp = fcmp ult double %a, 0.000000e+00
+  %z.y = select i1 %cmp, double %z, double %y
+  ret double %z.y
+}
+
+; CHECK-LABEL: unsafe_math_on:
+define double @unsafe_math_on(double %a, double %y, double %z) #1 #3 {
+entry:
+; SAFE-NOT:   fsel
+; UNSAFE-NOT: fsel
+  %cmp = fcmp ult double %a, 0.000000e+00
+  %z.y = select i1 %cmp, double %z, double %y
+  ret double %z.y
+}
+
+; CHECK-LABEL: default2:
+define double @default2(double %a, double %y, double %z) {
+; SAFE-NOT:  fsel
+; UNSAFE:    fsel
+  %cmp = fcmp ult double %a, 0.000000e+00
+  %z.y = select i1 %cmp, double %z, double %y
+  ret double %z.y
+}
+
+attributes #0 = { "no-infs-fp-math"="false" }
+attributes #1 = { "no-nans-fp-math"="false" }
+
+attributes #2 = { "no-infs-fp-math"="false" }
+attributes #3 = { "no-infs-fp-math"="true" }

llvm/trunk/test/CodeGen/X86/change-unsafe-fp-math.ll

+; Check that we can enable/disable UnsafeFPMath via function attributes.  An
+; attribute on one function should not magically apply to the next one.
+
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown \
+; RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=SAFE
+
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -enable-unsafe-fp-math \
+; RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=UNSAFE
+
+; The div in these functions should be converted to a mul when unsafe-fp-math
+; is enabled.
+
+; CHECK-LABEL: unsafe_fp_math_default0:
+define double @unsafe_fp_math_default0(double %x) {
+; SAFE:      divsd
+; UNSAFE:    mulsd
+  %div = fdiv double %x, 2.0
+  ret double %div
+}
+
+; CHECK-LABEL: unsafe_fp_math_off:
+define double @unsafe_fp_math_off(double %x) #0 {
+; SAFE:      divsd
+; UNSAFE:    divsd
+  %div = fdiv double %x, 2.0
+  ret double %div
+}
+
+; CHECK-LABEL: unsafe_fp_math_default1:
+define double @unsafe_fp_math_default1(double %x) {
+; With unsafe math enabled, can change this div to a mul.
+; SAFE:      divsd
+; UNSAFE:    mulsd
+  %div = fdiv double %x, 2.0
+  ret double %div
+}
+
+; CHECK-LABEL: unsafe_fp_math_on:
+define double @unsafe_fp_math_on(double %x) #1 {
+; SAFE:      mulsd
+; UNSAFE:    mulsd
+  %div = fdiv double %x, 2.0
+  ret double %div
+}
+
+; CHECK-LABEL: unsafe_fp_math_default2:
+define double @unsafe_fp_math_default2(double %x) {
+; With unsafe math enabled, can change this div to a mul.
+; SAFE:      divsd
+; UNSAFE:    mulsd
+  %div = fdiv double %x, 2.0
+  ret double %div
+}
+
+attributes #0 = { "unsafe-fp-math"="false" }
+attributes #1 = { "unsafe-fp-math"="true" }