[FPEnv][Clang][Driver] Disable constrained floating point on targets lacking support.

[kpn]

Summary

We currently have strict floating point/constrained floating point enabled for all targets. Constrained SDAG nodes get converted to the regular ones before reaching the target layer. In theory this should be fine.

However, the changes are exposed to users through multiple clang options already in use in the field, and the changes are _completely_ _untested_ on almost all of our targets. Bugs have already been found, like "https://bugs.llvm.org/show_bug.cgi?id=45274".

This patch disables constrained floating point options in clang everywhere except X86 and SystemZ. A warning will be printed when this happens.

[kpn]

This currently triggers a failure in test "Clang :: Misc/warning-flags.c". Suggestions on how to fix that correctly are welcome.

[efriedma]

Clang :: Misc/warning-flags.c

Like the file says, all warnings should be controlled by a -W flag. If you're going to add a new warning, you need a corresponding flag.

[arsenm]

If this chooses to just blindly ignore the setting, how is development supposed to be done on other targets? I'd prefer if this behaved more like -fglobal-isel, where it warns on unsupported targets but doesn't decide to ignore you

[efriedma]

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

[arsenm]

In D80952#2067563, @efriedma wrote:

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

Could we get an -fexperimental-something flag to override this behavior then?

[kpn]

In D80952#2067643, @arsenm wrote:

In D80952#2067563, @efriedma wrote:

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

Could we get an -fexperimental-something flag to override this behavior then?

The vast majority of the work needed is in llvm and not clang. Most if not all testing is done with the llc executable. There is some work in clang's CGBuiltins.cpp, but that's pretty small compared to llvm. And with this patch the enabling of the clang support is only one line. That seems cheaper than yet another command line flag. Someone doing development of the clang parts is going to be building clang anyway so I don't see how a one line change is much of a burden.

The flags for strict fp and rounding are already pretty complicated already. I'd hate to make it even worse with yet another flag just to avoid having a developer have to carry one line in one source file. And when clang+llvm is ready for a target that one line would be needed _anyway_. So what would a new -fexperimental-something flag gain us?

Also, I'm not sure that the constrained intrinsics will ever be implemented on every last one of our targets. So a new flag would complicate matters basically forever.

[steven.zhang]

The PowerPC backend is also adding the constraint float point support and almost done.

[kpn]

Added new warnings to a new group "unsupported-floating-point-opt". The warning can be disabled. The disabling of constrained floating point is unaffected by the status of the warning.

I added PowerPC to the list of targets that are _not_ disabled by this patch since that target is close to parity with X86 and SystemZ.

Is there anything left?

[kpn]

Ping?

[wristow]

clang/test/CodeGen/fp-strictfp.cpp

2

Need to remove the "tee" command in the pipe sequence.

[kpn]

Remove debugging command left in accidentally. Rebase.

[kpn]

Ping. I'm really hoping to get this into 11. Otherwise we're going multiple releases with options that people already use causing crashes on most architectures.

[efriedma]

I'm not sure what the status is of constrained fp on arm/aarch64; some patches went in, but I'm not sure what sort of testing was done. @john.brawn @nikic ?

Otherwise, I think this patch makes sense.

[nemanjai]

As far as I'm concerned, this is fine for now. We can remove these once all in-tree target have implemented their support.
LGTM but maybe give a couple of days for others to chime in.

clang/lib/Basic/Targets/PPC.h

86

I don't think we need this for now. Close is not quite there. @steven.zhang I would prefer that we initially turn this off and only flip it on once the support is complete.
Also, is the support that is currently under development for both 32 and 64 bit architectures? If it is 64 bit only, then we can enable it only there once it is done.

[steven.zhang]

clang/lib/Basic/Targets/PPC.h

86

Yes, it hasn't been finished yet. Agree that we can turn on it later when it is done.

[thakis]

This seems to break tests everywhere, eg http://45.33.8.238/linux/22152/step_7.txt or
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/11251

Please take a look and revert if the fix takes a while.

[kpn]

Already on it. I hope I got it now.

[MaskRay]

Note also that @arsenm is still a blocking reviewer. It is generally expected that all feedback is acknowledged. @kpn you should probably have waited for @arsenm to explicitly clear the blocker.

[arsenm]

In D80952#2133693, @MaskRay wrote:

Note also that @arsenm is still a blocking reviewer. It is generally expected that all feedback is acknowledged. @kpn you should probably have waited for @arsenm to explicitly clear the blocker.

I think this is one of the finer points of phabricator usage that are generally ignored.

I'd still like to have a way to force this on unhandled targets, but I don't care that much about this

[kpn]

Add the -fexperimental-strict-floating-point flag to enable on hosts that are not marked as supporting strict FP yet. Add test and documentation.

Update tests to use the new flag. This eliminates the XFAIL lines and should keep the tests running like before.

Hopefully this will work for 11.

[MaskRay]

LG with some nits

clang/lib/Frontend/CompilerInvocation.cpp

3287 ↗

(On Diff #276166)

Delete redundant braces

clang/test/CodeGen/fp-strictfp.cpp

13

Append a punctuation, i.e. _Z12fp_precise_1fff:

18

Delete trailing empty lines

[kpn]

Thanks for the reviews and the fast turnaround! I do appreciate it!

[dang]

clang/include/clang/Driver/Options.td

1246 ↗

(On Diff #277004)

A bit late to the party, but can you mark this as MarshallingInfoFlag<"LangOpts->ExpStrictFP", "false> so that if (Args.hasArg(OPT_fexperimental_strict_floating_point)) Opts.ExpStrictFP = true; in CompilerInvocation.cpp gets generated automatically, we also get serializing the option for free this way.

[kpn]

clang/include/clang/Driver/Options.td

1246 ↗

(On Diff #277004)

Assuming this patch makes it to the afternoon, how about I open a new review for this?

[dang]

clang/include/clang/Driver/Options.td

1246 ↗

(On Diff #277004)

Sure no problem

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def warn_fe_backend_plugin: Warning<"%0">, BackendInfo, InGroup<BackendPlugin>;
 def err_fe_backend_plugin: Error<"%0">, BackendInfo;
 def remark_fe_backend_plugin: Remark<"%0">, BackendInfo, InGroup<RemarkBackendPlugin>;
 def note_fe_backend_plugin: Note<"%0">, BackendInfo;
 
 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 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; "

clang/include/clang/Basic/DiagnosticGroups.td

def FloatConversion :
   DiagGroup<"float-conversion", [FloatOverflowConversion,
                                  FloatZeroConversion]>;
 
 def FrameAddress : DiagGroup<"frame-address">;
 def DoublePromotion : DiagGroup<"double-promotion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
 def UnsupportedNan : DiagGroup<"unsupported-nan">;
 def UnsupportedAbs : DiagGroup<"unsupported-abs">;
+def UnsupportedFPOpt : DiagGroup<"unsupported-floating-point-opt">;
 def UnsupportedCB : DiagGroup<"unsupported-cb">;
 def UnsupportedGPOpt : DiagGroup<"unsupported-gpopt">;
 def UnsupportedTargetOpt : DiagGroup<"unsupported-target-opt">;
 def NonLiteralNullConversion : DiagGroup<"non-literal-null-conversion">;
 def NullConversion : DiagGroup<"null-conversion">;
 def ImplicitConversionFloatingPointToBool :
   DiagGroup<"implicit-conversion-floating-point-to-bool">;
 def ObjCLiteralConversion : DiagGroup<"objc-literal-conversion">;

clang/include/clang/Basic/TargetInfo.h

protected:
   bool TLSSupported;
   bool VLASupported;
   bool NoAsmVariants;  // True if {|} are normal characters.
   bool HasLegalHalfType; // True if the backend supports operations on the half
                          // LLVM IR type.
   bool HasFloat128;
   bool HasFloat16;
   bool HasBFloat16;
+  bool HasStrictFP;
 
   unsigned char MaxAtomicPromoteWidth, MaxAtomicInlineWidth;
   unsigned short SimdDefaultAlign;
   std::unique_ptr<llvm::DataLayout> DataLayout;
   const char *MCountName;
   unsigned char RegParmMax, SSERegParmMax;
   TargetCXXABI TheCXXABI;
   const LangASMap *AddrSpaceMap;
public:
   virtual bool hasFloat128Type() const { return HasFloat128; }
 
   /// Determine whether the _Float16 type is supported on this target.
   virtual bool hasFloat16Type() const { return HasFloat16; }
 
   /// Determine whether the _BFloat16 type is supported on this target.
   virtual bool hasBFloat16Type() const { return HasBFloat16; }
 
+  /// Determine whether constrained floating point is supported on this target.
+  virtual bool hasStrictFP() const { return HasStrictFP; }
+
   /// Return the alignment that is suitable for storing any
   /// object with a fundamental alignment requirement.
   unsigned getSuitableAlign() const { return SuitableAlign; }
 
   /// Return the default alignment for __attribute__((aligned)) on
   /// this target, to be used if no alignment value is specified.
   unsigned getDefaultAlignForAttributeAligned() const {
     return DefaultAlignForAttributeAligned;

clang/lib/Basic/TargetInfo.cpp

TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
   BigEndian = !T.isLittleEndian();
   TLSSupported = true;
   VLASupported = true;
   NoAsmVariants = false;
   HasLegalHalfType = false;
   HasFloat128 = false;
   HasFloat16 = false;
   HasBFloat16 = false;
+  HasStrictFP = false;
   PointerWidth = PointerAlign = 32;
   BoolWidth = BoolAlign = 8;
   IntWidth = IntAlign = 32;
   LongWidth = LongAlign = 32;
   LongLongWidth = LongLongAlign = 64;
 
   // Fixed point default bit widths
   ShortAccumWidth = ShortAccumAlign = 16;

clang/lib/Basic/Targets/PPC.h

 
 public:
   PPCTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
       : TargetInfo(Triple) {
     SuitableAlign = 128;
     SimdDefaultAlign = 128;
     LongDoubleWidth = LongDoubleAlign = 128;
     LongDoubleFormat = &llvm::APFloat::PPCDoubleDouble();
+
+    HasStrictFP = true;
   }
 
   // Set the language option for altivec based on our value.
   void adjust(LangOptions &Opts) override;
 
   // Note: GCC recognizes the following additional cpus:
   //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
   //  821, 823, 8540, e300c2, e300c3, e500mc64, e6500, 860, cell, titan, rs64.

clang/lib/Basic/Targets/SystemZ.h

SystemZTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
     PointerWidth = PointerAlign = 64;
     LongDoubleWidth = 128;
     LongDoubleAlign = 64;
     LongDoubleFormat = &llvm::APFloat::IEEEquad();
     DefaultAlignForAttributeAligned = 64;
     MinGlobalAlign = 16;
     resetDataLayout("E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64");
     MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+    HasStrictFP = true;
   }
 
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   ArrayRef<const char *> getGCCRegNames() const override;

clang/lib/Basic/Targets/X86.h

protected:
 
   enum FPMathKind { FP_Default, FP_SSE, FP_387 } FPMath = FP_Default;
 
 public:
   X86TargetInfo(const llvm::Triple &Triple, const TargetOptions &)
       : TargetInfo(Triple) {
     LongDoubleFormat = &llvm::APFloat::x87DoubleExtended();
     AddrSpaceMap = &X86AddrSpaceMap;
+    HasStrictFP = true;
   }
 
   const char *getLongDoubleMangling() const override {
     return LongDoubleFormat == &llvm::APFloat::IEEEquad() ? "g" : "e";
   }
 
   unsigned getFloatEvalMethod() const override {
     // X87 evaluates with 80 bits "long double" precision.

clang/lib/Frontend/CompilerInstance.cpp

if ((getLangOpts().CUDA || getLangOpts().OpenMPIsDevice ||
     if (getFrontendOpts().AuxTargetCPU)
       TO->CPU = getFrontendOpts().AuxTargetCPU.getValue();
     if (getFrontendOpts().AuxTargetFeatures)
       TO->FeaturesAsWritten = getFrontendOpts().AuxTargetFeatures.getValue();
     TO->HostTriple = getTarget().getTriple().str();
     setAuxTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), TO));
   }
 
+  if (!getTarget().hasStrictFP()) {
+    if (getLangOpts().getFPRoundingMode() !=
+        llvm::RoundingMode::NearestTiesToEven) {
+      getDiagnostics().Report(diag::warn_fe_backend_unsupported_fp_rounding);
+      getLangOpts().setFPRoundingMode(llvm::RoundingMode::NearestTiesToEven);
+    }
+    if (getLangOpts().getFPExceptionMode() != LangOptions::FPE_Ignore) {
+      getDiagnostics().Report(diag::warn_fe_backend_unsupported_fp_exceptions);
+      getLangOpts().setFPExceptionMode(LangOptions::FPE_Ignore);
+    }
+    // FIXME: can we disable FEnvAccess?
+  }
+
   // Inform the target of the language options.
   //
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   getTarget().adjust(getLangOpts());
 
   // Adjust target options based on codegen options.
   getTarget().adjustTargetOptions(getCodeGenOpts(), getTargetOpts());

clang/test/CodeGen/aarch64-neon-misc-constrained.c

 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 // RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon \
 // RUN:  -ffp-exception-behavior=strict \
 // RUN:  -disable-O0-optnone -fallow-half-arguments-and-returns -S -o - %s \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 
 // REQUIRES: aarch64-registered-target
 
+// Disabled until constrained floating point is implemented for arm64.
+// XFAIL: *
+
 // Test new aarch64 intrinsics and types but constrained
 
 #include <arm_neon.h>
 
 // COMMON-LABEL: test_vrndaq_f64
 // COMMONIR:   [[TMP0:%.*]] = bitcast <2 x double> %a to <16 x i8>
 // UNCONSTRAINED: [[VRNDA1_I:%.*]] = call <2 x double> @llvm.round.v2f64(<2 x double> %a)
 // CONSTRAINED:   [[VRNDA1_I:%.*]] = call <2 x double> @llvm.experimental.constrained.round.v2f64(<2 x double> %a, metadata !"fpexcept.strict")

clang/test/CodeGen/aarch64-neon-scalar-x-indexed-elem-constrained.c

 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 // RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon -target-cpu cyclone \
 // RUN: -ffp-exception-behavior=strict \
 // RUN: -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | llc -o=- - \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 
 // REQUIRES: aarch64-registered-target
 
+// Disabled until constrained floating point is implemented for arm64.
+// XFAIL: *
+
 // Test new aarch64 intrinsics and types but constrained
 
 #include <arm_neon.h>
 
 // COMMON-LABEL: test_vfmas_lane_f32
 // COMMONIR:        [[EXTRACT:%.*]] = extractelement <2 x float> %c, i32 1
 // UNCONSTRAINED:   [[TMP2:%.*]] = call float @llvm.fma.f32(float %b, float [[EXTRACT]], float %a)
 // CONSTRAINED:     [[TMP2:%.*]] = call float @llvm.experimental.constrained.fma.f32(float %b, float [[EXTRACT]], float %a, metadata !"round.tonearest", metadata !"fpexcept.strict")

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c

 // RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon -target-feature +fullfp16 -target-feature +v8.2a\
 // RUN: -ffp-exception-behavior=strict \
 // RUN: -fallow-half-arguments-and-returns -flax-vector-conversions=none -S -disable-O0-optnone -emit-llvm -o - %s \
 // RUN: | opt -S -mem2reg | llc -o=- - \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 
 // REQUIRES: aarch64-registered-target
 
+// Disabled until constrained floating point is implemented for arm64.
+// XFAIL: *
+
 #include <arm_neon.h>
 
 // COMMON-LABEL: test_vsqrt_f16
 // UNCONSTRAINED:  [[SQR:%.*]] = call <4 x half> @llvm.sqrt.v4f16(<4 x half> %a)
 // CONSTRAINED:    [[SQR:%.*]] = call <4 x half> @llvm.experimental.constrained.sqrt.v4f16(<4 x half> %a, metadata !"round.tonearest", metadata !"fpexcept.strict")
 // CHECK-ASM:      fsqrt v{{[0-9]+}}.4h, v{{[0-9]+}}.4h
 // COMMONIR:       ret <4 x half> [[SQR]]
 float16x4_t test_vsqrt_f16(float16x4_t a) {

clang/test/CodeGen/arm-neon-directed-rounding-constrained.c

 // RUN:     opt -S -mem2reg | llc -o=- - | FileCheck -check-prefixes=COMMON,CHECK-ASM32 %s
 // RUN: %clang_cc1 -triple arm64-linux-gnueabihf -target-feature +neon \
 // RUN:     -ffp-exception-behavior=strict \
 // RUN:     -ffreestanding -disable-O0-optnone -emit-llvm %s -o - | \
 // RUN:     opt -S -mem2reg | llc -o=- - | FileCheck -check-prefixes=COMMON,CHECK-ASM64 %s
 
 // REQUIRES: arm-registered-target,aarch64-registered-target
 
+// Disabled until constrained floating point is implemented for arm64.
+// XFAIL: *
+
 #include <arm_neon.h>
 
 // COMMON-LABEL: test_vrndi_f32
 // COMMONIR:      [[TMP0:%.*]] = bitcast <2 x float> %a to <8 x i8>
 // UNCONSTRAINED: [[VRNDI1_I:%.*]] = call <2 x float> @llvm.nearbyint.v2f32(<2 x float> %a)
 // CONSTRAINED:   [[VRNDI1_I:%.*]] = call <2 x float> @llvm.experimental.constrained.nearbyint.v2f32(<2 x float> %a, metadata !"round.tonearest", metadata !"fpexcept.strict")
 // CHECK-ASM32:   vrintr.f32 s{{[0-9]+}}, s{{[0-9]+}}
 // CHECK-ASM32:   vrintr.f32 s{{[0-9]+}}, s{{[0-9]+}}

clang/test/CodeGen/arm64-vrnd-constrained.c

 // RUN: %clang_cc1 -triple arm64-apple-ios7 -target-feature +neon -ffreestanding -flax-vector-conversions=none -emit-llvm -o - %s \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=COMMONIR --check-prefix=UNCONSTRAINED %s
 // RUN: %clang_cc1 -triple arm64-apple-ios7 -target-feature +neon -ffreestanding -flax-vector-conversions=none -ffp-exception-behavior=strict -emit-llvm -o - %s \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=COMMONIR --check-prefix=CONSTRAINED %s
 // RUN: %clang_cc1 -triple arm64-apple-ios7 -target-feature +neon -ffreestanding -flax-vector-conversions=none -emit-llvm -o - %s | llc -o=- - \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 // RUN: %clang_cc1 -triple arm64-apple-ios7 -target-feature +neon -ffreestanding -flax-vector-conversions=none -ffp-exception-behavior=strict -emit-llvm -o - %s | llc -o=- - \
 // RUN: | FileCheck --check-prefix=COMMON --check-prefix=CHECK-ASM %s
 
 // REQUIRES: aarch64-registered-target
 
+// Disabled until constrained floating point is implemented for arm64.
+// XFAIL: *
+
 #include <arm_neon.h>
 
 float64x2_t rnd5(float64x2_t a) { return vrndq_f64(a); }
 // COMMON-LABEL: rnd5
 // UNCONSTRAINED: call <2 x double> @llvm.trunc.v2f64(<2 x double>
 // CONSTRAINED:   call <2 x double> @llvm.experimental.constrained.trunc.v2f64(<2 x double>
 // CHECK-ASM:     frintz.2d v{{[0-9]+}}, v{{[0-9]+}}
 

clang/test/CodeGen/fp-strictfp.cpp

+// RUN: %clang_cc1 -triple mips64-linux-gnu -frounding-math -ffp-exception-behavior=strict -O2 -verify=rounding,exception -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple mips64-linux-gnu -ffp-exception-behavior=strict -O2 -verify=exception -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple mips64-linux-gnu -frounding-math -O2 -verify=rounding -emit-llvm -o - %s | FileCheck %s
+//
+// Verify that constrained intrinsics are not used.
+// As more targets gain support for constrained intrinsics the triple
+// in this test will need to change.
+
+// rounding-warning@* {{overriding currently unsupported rounding mode on this target}}
+// exception-warning@* {{overriding currently unsupported use of floating point exceptions on this target}}
+float fp_precise_1(float a, float b, float c) {
+// CHECK: _Z12fp_precise_1fff
+// CHECK: %[[M:.+]] = fmul float{{.*}}
+// CHECK: fadd float %[[M]], %c
+  return a * b + c;
+}
+
+