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

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel added subscribers: kpn, sepavloff, andrew.w.kaylor.
spatel added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sanjay Patel via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8c7f2a4f8719: [CodeGen] use saturating FP casts when 
compiling with no-strict-float-cast… (authored by spatel).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D115804?vs=394571=394850#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/no-junk-ftrunc.c


Index: clang/test/CodeGen/no-junk-ftrunc.c
===
--- clang/test/CodeGen/no-junk-ftrunc.c
+++ clang/test/CodeGen/no-junk-ftrunc.c
@@ -1,14 +1,22 @@
 // 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;
 }
-
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -1240,7 +1240,18 @@
 
   if (isa(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");
   }


Index: clang/test/CodeGen/no-junk-ftrunc.c
===
--- clang/test/CodeGen/no-junk-ftrunc.c
+++ clang/test/CodeGen/no-junk-ftrunc.c
@@ -1,14 +1,22 @@
 // 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;
 }
-
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -1240,7 +1240,18 @@
 
   if (isa(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");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

This sounds like a great fix to me! :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

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
  }


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115804/new/

https://reviews.llvm.org/D115804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel created this revision.
spatel added reviewers: MatzeB, neildhar, nikic, aqjune, dmgreen.
Herald added a subscriber: mcrosier.
spatel requested review of this revision.

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.


https://reviews.llvm.org/D115804

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/no-junk-ftrunc.c


Index: clang/test/CodeGen/no-junk-ftrunc.c
===
--- clang/test/CodeGen/no-junk-ftrunc.c
+++ clang/test/CodeGen/no-junk-ftrunc.c
@@ -1,14 +1,23 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // 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;
 }
-
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -1240,7 +1240,18 @@
 
   if (isa(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");
   }


Index: clang/test/CodeGen/no-junk-ftrunc.c
===
--- clang/test/CodeGen/no-junk-ftrunc.c
+++ clang/test/CodeGen/no-junk-ftrunc.c
@@ -1,14 +1,23 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // 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;
 }
-
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -1240,7 +1240,18 @@
 
   if (isa(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,