[CodeGen] use saturating FP casts when compiling with "no-strict-float-cast-overflow"

[spatel]

Summary

We got an unintended consequence of the optimizer getting smarter when compiling in a non-standard mode, and there's no good way to inhibit those optimizations at a later stage. The test is based on an example linked from D92270.

We allow the "no-strict-float-cast-overflow" exception to normal C cast rules to preserve legacy code that does not expect overflowing casts from FP to int to produce UB. See D46236 for details.

[nikic]

Looks reasonable. Making float to int cast well defined is exactly why these intrinsics exist. Should we also drop the "string-float-cast-overflow" attribute, as UB is now prevented in a different way?

[spatel]

In D115804#3195002, @nikic wrote:

Looks reasonable. Making float to int cast well defined is exactly why these intrinsics exist. Should we also drop the "string-float-cast-overflow" attribute, as UB is now prevented in a different way?

Yes, that sounds like a good enhancement. The only in-tree use of the attribute is in DAGCombiner. Make that a follow-up patch or two since it crosses into LLVM?
If we care about perf of the generated code in those cases, then we need to add something to the optimizer or codegen to recognize patterns like this:

define float @s(float %x) {
  %i = call i32 @llvm.fptosi.sat.i32.f32(float %x)
  %f = sitofp i32 %i to float
  ret float %f
}

[aqjune]

This sounds like a great fix to me! :)

[nikic]

LGTM

In D115804#3195158, @spatel wrote:

In D115804#3195002, @nikic wrote:

Looks reasonable. Making float to int cast well defined is exactly why these intrinsics exist. Should we also drop the "string-float-cast-overflow" attribute, as UB is now prevented in a different way?

Yes, that sounds like a good enhancement. The only in-tree use of the attribute is in DAGCombiner. Make that a follow-up patch or two since it crosses into LLVM?

Followup sounds good.

[craig.topper]

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

[spatel]

In D115804#3201044, @craig.topper wrote:

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

Not sure. cc'ing @kpn @sepavloff @andrew.w.kaylor
The saturating intrinsics implement non-standard behavior for C languages AFAIK, so we might want to warn if someone tries to use "-fno-strict-float-cast-overflow" and "-ffp-exception-behavior=strict" at the same time? Or we try to support that corner case by adding even more FP intrinsics?

[sepavloff]

In D115804#3201681, @spatel wrote:

In D115804#3201044, @craig.topper wrote:

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

Not sure. cc'ing @kpn @sepavloff @andrew.w.kaylor
The saturating intrinsics implement non-standard behavior for C languages AFAIK, so we might want to warn if someone tries to use "-fno-strict-float-cast-overflow" and "-ffp-exception-behavior=strict" at the same time? Or we try to support that corner case by adding even more FP intrinsics?

Conversion float->int depends on rounding mode. At least on some ML cores this conversion is made with saturating semantics. So ability to specify rounding mode would be useful for such targets.

[kpn]

In D115804#3202479, @sepavloff wrote:

In D115804#3201681, @spatel wrote:

In D115804#3201044, @craig.topper wrote:

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

Not sure. cc'ing @kpn @sepavloff @andrew.w.kaylor
The saturating intrinsics implement non-standard behavior for C languages AFAIK, so we might want to warn if someone tries to use "-fno-strict-float-cast-overflow" and "-ffp-exception-behavior=strict" at the same time? Or we try to support that corner case by adding even more FP intrinsics?

Conversion float->int depends on rounding mode. At least on some ML cores this conversion is made with saturating semantics. So ability to specify rounding mode would be useful for such targets.

What about the idea of using operand bundles? I think the idea there was to avoid having to add new intrinsics for every target-specific floating point intrinsic. But maybe the idea will work here as well?

I think we definitely need to warn when -ffp-exception-behavior=strict or =maytrap is given and incompatible options are also used with it. In that case we should also be disabling the -ffp-exception-behavior option and the related #pragmas plus warning about the disabling. We already disable+warn when our backend doesn't support strictfp, we can do the same here.

If someone in the future wants to implement the support then that'll be nice. But in the meantime we shouldn't be silently letting people use strictfp plus a contradictory option.

clang/lib/CodeGen/CGExprScalar.cpp

if (isa<llvm::IntegerType>(DstElementTy))
       return Builder.CreateIntCast(Src, DstTy, InputSigned, "conv");
     if (InputSigned)
       return Builder.CreateSIToFP(Src, DstTy, "conv");
     return Builder.CreateUIToFP(Src, DstTy, "conv");
   }
 
   if (isa<llvm::IntegerType>(DstElementTy)) {
     assert(SrcElementTy->isFloatingPointTy() && "Unknown real conversion");
-    if (DstElementType->isSignedIntegerOrEnumerationType())
+    bool IsSigned = DstElementType->isSignedIntegerOrEnumerationType();
+
+    // If we can't recognize overflow as undefined behavior, assume that
+    // overflow saturates. This protects against normal optimizations if we are
+    // compiling with non-standard FP semantics.
+    if (!CGF.CGM.getCodeGenOpts().StrictFloatCastOverflow) {
+      llvm::Intrinsic::ID IID =
+          IsSigned ? llvm::Intrinsic::fptosi_sat : llvm::Intrinsic::fptoui_sat;
+      return Builder.CreateCall(CGF.CGM.getIntrinsic(IID, {DstTy, SrcTy}), Src);
+    }
+
+    if (IsSigned)
       return Builder.CreateFPToSI(Src, DstTy, "conv");
     return Builder.CreateFPToUI(Src, DstTy, "conv");
   }
 
   if (DstElementTy->getTypeID() < SrcElementTy->getTypeID())
     return Builder.CreateFPTrunc(Src, DstTy, "conv");
   return Builder.CreateFPExt(Src, DstTy, "conv");
 }

clang/test/CodeGen/no-junk-ftrunc.c

 // RUN: %clang_cc1 -S -fno-strict-float-cast-overflow %s -emit-llvm -o - | FileCheck %s --check-prefix=NOSTRICT
+
+// When compiling with non-standard semantics, use intrinsics to inhibit the optimizer.
+
 // NOSTRICT-LABEL: main
+// NOSTRICT: call i32 @llvm.fptosi.sat.i32.f64
+// NOSTRICT: call i32 @llvm.fptoui.sat.i32.f64
 // NOSTRICT: attributes #0 = {{.*}}"strict-float-cast-overflow"="false"{{.*}}
 
 // The workaround attribute is not applied by default.
 
 // RUN: %clang_cc1 -S %s -emit-llvm -o - | FileCheck %s --check-prefix=STRICT
 // STRICT-LABEL: main
+// STRICT: = fptosi
+// STRICT: = fptoui
 // STRICT-NOT: strict-float-cast-overflow
 
+
 int main() {
-  return 0;
+  double d = 1e20;
+  return (int)d != 1e20 && (unsigned)d != 1e20;
 }
-