[Driver, CodeGen] add options to enable/disable an FP cast optimization

[spatel]

Summary

As discussed in the post-commit thread for:
rL330437 ( http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180423/545906.html )

We need a way to opt-out of a float-to-int-to-float cast optimization because too much existing code relies on the platform-specific undefined result of those casts when the float-to-int overflows.

I speculatively committed the LLVM changes associated with adding this function attribute, but I'll change the name/implementation if there's a better alternative:
rL330947
rL330950
rL330951

Also as suggested, I changed the LLVM doc to mention the specific sanitizer flag that catches this problem:
rL330958

I tested the end-to-end results on x86 and see the expected outcome: 'roundss' is no longer produced in place of cvttss2si + cvtsi2ss with default optimization.

[lebedev.ri]

test/CodeGen/no-junk-ftrunc.c

1

For a good measure, i'd add one more RUN line to test that it is currently the default.
(Yes, i noticed that it is already tested in test/Driver/fast-math.c)

[spatel]

test/CodeGen/no-junk-ftrunc.c

1

The driver alone is handling the default setting, so it passes this flag to the front-end only when we're going to disable the transform. Ie, the driver eats "-fno-fp-cast-overflow-workaround" and sends nothing in that case to the front-end.

So there's not currently any case where the function attribute will be "fp-cast-overflow-workaround=false", but I left that as a possibility in case we decide to lift the limit at a finer granularity (scalar vs. vector etc).

I may be misunderstanding the question/suggestion - do we want the front-end to independently have a default setting?

[jgorbe]

Can't comment much on the patch itself (I'm still not very familiar with the codebase, I'll leave that to the other reviewers), but thanks a lot for responding so quickly! :)

[chandlerc]

docs/UsersManual.rst

1260–1265

I would phrase this the other way around (and I think the flag name is already phrased the other way around?):

"""
Enable a workaround for incorrect code that casts floating point values to integers where the floating point value is not representable in the integer type. This code is incorrect according to the language standard, but this flag will attempt to generate code to cause <insert expected behavior with the flag enabled>.
"""

Essentially, this should be more like '-fwrapv'. Also, I think the default should be what the specification says. People can explicitly pass this flag if their code is broken in this way.

[spatel]

docs/UsersManual.rst

1260–1265

Ah - did I misinterpret the earlier comments?

I thought we need to have the work-around 'on' by default as the immediate fix for broken programs?

[chandlerc]

docs/UsersManual.rst

1260–1265

Maybe others feel strongly about the default. I'm happy to explicitly pass a flag for our code until we get it fixed here.

I would suggest starting by adding the flag to toggle the behavior but not changing the default (which as of now is 'optimize, no workaround') and then we can invert the default if we get enough feedback that this is causing users problems.

Either way, I'd word this as suggested above.

[scanon]

I like Chandler's wording. Something like:

"... this flag will attempt to cause <clang to generate code as though the result of such conversions were defined to be an unspecified integer value.>"

[spatel]

Patch updated:

1. Improve the documentation language - more suggestions welcome!
2. Change the default setting so the work-around is 'off' (ie, by default assume source is compliant and optimize accordingly).
3. Remove the 'no' version of the flag. Given the change in the default, this seems more natural to me, and it simplifies the patch/tests...but I might have been too pessimistic before and this is too optimistic? Let me know...

[chandlerc]

In D46135#1080108, @spatel wrote:

3. Remove the 'no' version of the flag. Given the change in the default, this seems more natural to me, and it simplifies the patch/tests...but I might have been too pessimistic before and this is too optimistic? Let me know...

Please keep the no- version so that the flag can be toggled by appending to the command line.

test/CodeGen/no-junk-ftrunc.c

1

How about a test that checks the attribute is *absent* in the default mode then?

I think it's useful to have the both sides of the test, however a particular side is spelled.

[spatel]

Patch updated:

1. Restore the 'no' option to allow toggling.
2. Add a RUN to the codegen test to show that the function attribute is not appended by default.

[chandlerc]

LGTM, thanks so much!

[spatel]

Should I add a bullet for this new flag/attribute to the clang release notes, the LLVM release, both? Or do we view this as temporary and not making it to the release?

[lebedev.ri]

In D46135#1081165, @spatel wrote:

Should I add a bullet for this new flag/attribute to the clang release notes, the LLVM release, both? Or do we view this as temporary and not making it to the release?

(Not everyone is always using trunk snapshots)
I'd guess it would be present for at least one release, so I would expect to see that in both the clang and llvm release notes.

[spatel]

In D46135#1081193, @lebedev.ri wrote:

(Not everyone is always using trunk snapshots)
I'd guess it would be present for at least one release, so I would expect to see that in both the clang and llvm release notes.

Thanks - sounds right to me:
rL331056
rL331059

docs/UsersManual.rst

.. option:: -fdenormal-fp-math=[values]
 
    Select which denormal numbers the code is permitted to require.
 
    Valid values are: ``ieee``, ``preserve-sign``, and ``positive-zero``,
    which correspond to IEEE 754 denormal numbers, the sign of a
    flushed-to-zero number is preserved in the sign of 0, denormals are
    flushed to positive zero, respectively.
 
+.. option:: -f[no-]fp-cast-overflow-workaround
+
+   Enable or disable a code generation optimization that may convert a
+   cast of a floating-point value to integer and back to floating-point
+   into the equivalent of the math libary's 'trunc()' function. This
+   optimization is disabled by default to avoid problems associated
+   with relying on the undefined behavior of an overflowing cast, but 
+   that limitation should be considered temporary. 
+
 .. option:: -fwhole-program-vtables
 
    Enable whole-program vtable optimizations, such as single-implementation
    devirtualization and virtual constant propagation, for classes with
    :doc:`hidden LTO visibility <LTOVisibility>`. Requires ``-flto``.
 
 .. option:: -fno-assume-sane-operator-new
 

include/clang/Driver/Options.td

 def : Flag<["-"], "fhonor-infinites">, Alias<fhonor_infinities>;
 def : Flag<["-"], "fno-honor-infinites">, Alias<fno_honor_infinities>;
 def ftrapping_math : Flag<["-"], "ftrapping-math">, Group<f_Group>, Flags<[CC1Option]>;
 def fno_trapping_math : Flag<["-"], "fno-trapping-math">, Group<f_Group>, Flags<[CC1Option]>;
 def ffp_contract : Joined<["-"], "ffp-contract=">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Form fused FP ops (e.g. FMAs): fast (everywhere)"
   " | on (according to FP_CONTRACT pragma, default) | off (never fuse)">, Values<"fast,on,off">;
 
+def ffp_cast_overflow_workaround : Flag<["-"],
+  "ffp-cast-overflow-workaround">, Group<f_Group>, Flags<[CC1Option]>;
+def fno_fp_cast_overflow_workaround : Flag<["-"],
+  "fno-fp-cast-overflow-workaround">, Group<f_Group>, Flags<[CC1Option]>;
+
 def ffor_scope : Flag<["-"], "ffor-scope">, Group<f_Group>;
 def fno_for_scope : Flag<["-"], "fno-for-scope">, Group<f_Group>;
 
 def frewrite_includes : Flag<["-"], "frewrite-includes">, Group<f_Group>,
   Flags<[CC1Option]>;
 def fno_rewrite_includes : Flag<["-"], "fno-rewrite-includes">, Group<f_Group>;
 
 def frewrite_imports : Flag<["-"], "frewrite-imports">, Group<f_Group>,

include/clang/Frontend/CodeGenOptions.def

 CODEGENOPT(NoInfsFPMath      , 1, 0) ///< Assume FP arguments, results not +-Inf.
 CODEGENOPT(NoSignedZeros     , 1, 0) ///< Allow ignoring the signedness of FP zero
 CODEGENOPT(Reassociate       , 1, 0) ///< Allow reassociation of FP math ops
 CODEGENOPT(ReciprocalMath    , 1, 0) ///< Allow FP divisions to be reassociated.
 CODEGENOPT(NoTrappingMath    , 1, 0) ///< Set when -fno-trapping-math is enabled.
 CODEGENOPT(NoNaNsFPMath      , 1, 0) ///< Assume FP arguments, results not NaN.
 CODEGENOPT(FlushDenorm       , 1, 0) ///< Allow FP denorm numbers to be flushed to zero
 CODEGENOPT(CorrectlyRoundedDivSqrt, 1, 0) ///< -cl-fp32-correctly-rounded-divide-sqrt
+
+/// Disable a float-to-int-to-float cast optimization to preserve behavior for
+/// source code that depends on existing -- but undefined -- behavior.
+CODEGENOPT(FPCastOverflowWorkaround, 1, 0)
+
 CODEGENOPT(UniformWGSize     , 1, 0) ///< -cl-uniform-work-group-size
 CODEGENOPT(NoZeroInitializedInBSS , 1, 0) ///< -fno-zero-initialized-in-bss.
 /// \brief Method of Objective-C dispatch to use.
 ENUM_CODEGENOPT(ObjCDispatchMethod, ObjCDispatchMethodKind, 2, Legacy)
 CODEGENOPT(OmitLeafFramePointer , 1, 0) ///< Set when -momit-leaf-frame-pointer is
                                         ///< enabled.
 
 VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.

lib/CodeGen/CGCall.cpp

FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
     if (!CodeGenOpts.FPDenormalMode.empty())
       FuncAttrs.addAttribute("denormal-fp-math", CodeGenOpts.FPDenormalMode);
 
     FuncAttrs.addAttribute("no-trapping-math",
                            llvm::toStringRef(CodeGenOpts.NoTrappingMath));
 
+    if (CodeGenOpts.FPCastOverflowWorkaround)
+      FuncAttrs.addAttribute("fp-cast-overflow-workaround", "true");
+
     // TODO: Are these all needed?
     // unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags.
     FuncAttrs.addAttribute("no-infs-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoInfsFPMath));
     FuncAttrs.addAttribute("no-nans-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoNaNsFPMath));
     FuncAttrs.addAttribute("unsafe-fp-math",
                            llvm::toStringRef(CodeGenOpts.UnsafeFPMath));

lib/Driver/ToolChains/Clang.cpp

static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
   // Handle __FINITE_MATH_ONLY__ similarly.
   if (!HonorINFs && !HonorNaNs)
     CmdArgs.push_back("-ffinite-math-only");
 
   if (const Arg *A = Args.getLastArg(options::OPT_mfpmath_EQ)) {
     CmdArgs.push_back("-mfpmath");
     CmdArgs.push_back(A->getValue());
   }
+
+  // Disable a codegen optimization for floating-point casts. This is only
+  // intended to be temporarily defaulted to 'true' to give users time to fix
+  // their code.
+  if (Args.hasFlag(options::OPT_ffp_cast_overflow_workaround,
+                   options::OPT_fno_fp_cast_overflow_workaround,
+                   true))
+    CmdArgs.push_back("-ffp-cast-overflow-workaround");
 }
 
 static void RenderAnalyzerOptions(const ArgList &Args, ArgStringList &CmdArgs,
                                   const llvm::Triple &Triple,
                                   const InputInfo &Input) {
   // Enable region store model by default.
   CmdArgs.push_back("-analyzer-store=region");
 

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.FlushDenorm = Args.hasArg(OPT_cl_denorms_are_zero);
   Opts.CorrectlyRoundedDivSqrt =
       Args.hasArg(OPT_cl_fp32_correctly_rounded_divide_sqrt);
   Opts.UniformWGSize =
       Args.hasArg(OPT_cl_uniform_work_group_size);
   Opts.Reciprocals = Args.getAllArgValues(OPT_mrecip_EQ);
   Opts.ReciprocalMath = Args.hasArg(OPT_freciprocal_math);
   Opts.NoTrappingMath = Args.hasArg(OPT_fno_trapping_math);
+  Opts.FPCastOverflowWorkaround = Args.hasArg(OPT_ffp_cast_overflow_workaround);
+
   Opts.NoZeroInitializedInBSS = Args.hasArg(OPT_mno_zero_initialized_in_bss);
   Opts.NumRegisterParameters = getLastArgIntValue(Args, OPT_mregparm, 0, Diags);
   Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);
   Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);
   Opts.EnableSegmentedStacks = Args.hasArg(OPT_split_stacks);
   Opts.RelaxAll = Args.hasArg(OPT_mrelax_all);
   Opts.IncrementalLinkerCompatible =
       Args.hasArg(OPT_mincremental_linker_compatible);

test/CodeGen/no-junk-ftrunc.c

+// RUN: %clang_cc1 -S -ffp-cast-overflow-workaround %s -emit-llvm -o - | FileCheck %s
+
+// CHECK-LABEL: main
+// CHECK: attributes #0 = {{.*}}"fp-cast-overflow-workaround"="true"{{.*}}
+
+int main() {
+  return 0;
+}

test/Driver/fast-math.c

 // CHECK-NO-REASSOC-NO_UNSAFE-MATH-NOT: "-mreassociate"
 // CHECK-NO-REASSOC-NO-UNSAFE-MATH-NOT: "-menable-unsafe-fp-math"
 // CHECK-NO-REASSOC-NO-UNSAFE-MATH: "-o"
 
 
 // RUN: %clang -### -ftrapping-math -fno-trapping-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-TRAPPING-MATH %s
 // CHECK-NO-TRAPPING-MATH: "-fno-trapping-math"
+
+// This isn't fast-math, but the option is handled in the same place as other FP params.
+// Last option wins, and the flag is passed by default (FIXME). 
+
+// RUN: %clang -### -ffp-cast-overflow-workaround -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPOV-WORKAROUND %s
+// CHECK-FPOV-WORKAROUND: "-cc1"
+// CHECK-FPOV-WORKAROUND: "-ffp-cast-overflow-workaround"
+
+// RUN: %clang -### -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPOV-WORKAROUND-DEFAULT %s
+// CHECK-FPOV-WORKAROUND-DEFAULT: "-cc1"
+// CHECK-FPOV-WORKAROUND-DEFAULT: "-ffp-cast-overflow-workaround"
+
+// RUN: %clang -### -fno-fp-cast-overflow-workaround -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FPOV-WORKAROUND %s
+// CHECK-NO-FPOV-WORKAROUND: "-cc1"
+// CHECK-NO-FPOV-WORKAROUND-NOT: "-ffp-cast-overflow-workaround"
+
+// RUN: %clang -### -ffp-cast-overflow-workaround -fno-fp-cast-overflow-workaround -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FPOV-WORKAROUND-OVERRIDE %s
+// CHECK-NO-FPOV-WORKAROUND-OVERRIDE: "-cc1"
+// CHECK-NO-FPOV-WORKAROUND-OVERRIDE-NOT: "-ffp-cast-overflow-workaround"
+