Skip to content

[flang] Use saturated intrinsic for floating point conversions #130686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Mar 12, 2025
Merged

[flang] Use saturated intrinsic for floating point conversions #130686

merged 3 commits into from
Mar 12, 2025

Conversation

ashermancinelli
Copy link
Contributor

The saturated floating point conversion intrinsics more closely match the semantics in the standard.

Case 2 of 16.9.100 is

INT (A [, KIND])
If A is of type real, there are two cases: if |A| < 1, INT (A) has the value 0; if |A| ≥ 1, INT (A) is the integer whose magnitude is the largest integer that does not exceed the magnitude of A and whose sign is the same as the sign of A.

Currently, converting a floating point value into an integer type too small to hold the constant will be converted to poison in opt, leaving us with garbage:

> cat t.f90
program main
  real(kind=16)   :: f
  integer(kind=4) :: i
  f=huge(f)
  i=f
  print *, i
end program main

# current upstream
> for i in `seq 10`; do; ./a.out; done
 -862156992
 -1497393344
 -739096768
 -1649494208
 1761228608
 -1959270592
 -746244288
 -1629194432
 -231217344
 382322496

With the saturated fptoui/fptosi intrinsics, we get the appropriate values

# mine
> flang -O2 ./t.f90 && ./a.out
 2147483647

> perl -e 'printf "%d\n", (2 ** 31) - 1'
2147483647

One notable difference: NaNs being converted to ints will become zero, unlike current flang (and other compilers).

@ashermancinelli
[fircg] Lower to saturated fp to int conversions
68ca207 
fptosi and fptoui llvm instructions become poison during optimization
when the conversion cannot be performed.

The standard mandates that a real converted to an int must be converted
to the largest integer that does not exceed the magnitude of the original
and keeps the same sign. The saturated floating point conversions match
these semantics more closely than the regular conversion instructions.
@ashermancinelli
Add unsigned conversion checks
302db78
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:codegen labels Mar 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 10, 2025 •
edited
Loading

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-codegen

Author: Asher Mancinelli (ashermancinelli)

Changes

The saturated floating point conversion intrinsics more closely match the semantics in the standard.

Case 2 of 16.9.100 is

> INT (A [, KIND])
> If A is of type real, there are two cases: if |A| < 1, INT (A) has the value 0; if |A| ≥ 1, INT (A) is the integer whose magnitude is the largest integer that does not exceed the magnitude of A and whose sign is the same as the sign of A.

Currently, converting a floating point value into an integer type too small to hold the constant will be converted to poison in opt, leaving us with garbage:

&gt; cat t.f90
program main
  real(kind=16)   :: f
  integer(kind=4) :: i
  f=huge(f)
  i=f
  print *, i
end program main

# current upstream
&gt; for i in `seq 10`; do; ./a.out; done
 -862156992
 -1497393344
 -739096768
 -1649494208
 1761228608
 -1959270592
 -746244288
 -1629194432
 -231217344
 382322496

With the saturated fptoui/fptosi intrinsics, we get the appropriate values

# mine
&gt; flang -O2 ./t.f90 &amp;&amp; ./a.out
 2147483647

&gt; perl -e 'printf "%d\n", (2 ** 31) - 1'
2147483647

One notable difference: NaNs being converted to ints will become zero, unlike current flang (and other compilers).

Full diff: https://github.com/llvm/llvm-project/pull/130686.diff

3 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+14-4)
  • (modified) flang/test/Fir/convert-to-llvm.fir (+13-5)
  • (added) flang/test/Integration/fp-convert.f90 (+236)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index a2743edd7844a..2302c08fae508 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -835,10 +835,20 @@ struct ConvertOpConversion : public fir::FIROpConversion<fir::ConvertOp> {
         return mlir::success();
       }
       if (mlir::isa<mlir::IntegerType>(toTy)) {
-        if (toTy.isUnsignedInteger())
-          rewriter.replaceOpWithNewOp<mlir::LLVM::FPToUIOp>(convert, toTy, op0);
-        else
-          rewriter.replaceOpWithNewOp<mlir::LLVM::FPToSIOp>(convert, toTy, op0);
+        // NOTE: We are checking the fir type here because toTy is an LLVM type
+        // which is signless, and we need to use the intrinsic that matches the
+        // sign of the output in fir.
+        if (toFirTy.isUnsignedInteger()) {
+          auto intrinsicName =
+              mlir::StringAttr::get(convert.getContext(), "llvm.fptoui.sat");
+          rewriter.replaceOpWithNewOp<mlir::LLVM::CallIntrinsicOp>(
+              convert, toTy, intrinsicName, op0);
+        } else {
+          auto intrinsicName =
+              mlir::StringAttr::get(convert.getContext(), "llvm.fptosi.sat");
+          rewriter.replaceOpWithNewOp<mlir::LLVM::CallIntrinsicOp>(
+              convert, toTy, intrinsicName, op0);
+        }
         return mlir::success();
       }
     } else if (mlir::isa<mlir::IntegerType>(fromTy)) {
diff --git a/flang/test/Fir/convert-to-llvm.fir b/flang/test/Fir/convert-to-llvm.fir
index c7037019ee701..2960528fb6c24 100644
--- a/flang/test/Fir/convert-to-llvm.fir
+++ b/flang/test/Fir/convert-to-llvm.fir
@@ -701,6 +701,10 @@ func.func @convert_from_float(%arg0 : f32) {
   %7 = fir.convert %arg0 : (f32) -> i16
   %8 = fir.convert %arg0 : (f32) -> i32
   %9 = fir.convert %arg0 : (f32) -> i64
+  %10 = fir.convert %arg0 : (f32) -> ui8
+  %11 = fir.convert %arg0 : (f32) -> ui16
+  %12 = fir.convert %arg0 : (f32) -> ui32
+  %13 = fir.convert %arg0 : (f32) -> ui64
   return
 }
 
@@ -711,11 +715,15 @@ func.func @convert_from_float(%arg0 : f32) {
 // CHECK:         %{{.*}} = llvm.fpext %[[ARG0]] : f32 to f64
 // CHECK:         %{{.*}} = llvm.fpext %[[ARG0]] : f32 to f80
 // CHECK:         %{{.*}} = llvm.fpext %[[ARG0]] : f32 to f128
-// CHECK:         %{{.*}} = llvm.fptosi %[[ARG0]] : f32 to i1
-// CHECK:         %{{.*}} = llvm.fptosi %[[ARG0]] : f32 to i8
-// CHECK:         %{{.*}} = llvm.fptosi %[[ARG0]] : f32 to i16
-// CHECK:         %{{.*}} = llvm.fptosi %[[ARG0]] : f32 to i32
-// CHECK:         %{{.*}} = llvm.fptosi %[[ARG0]] : f32 to i64
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptosi.sat"(%[[ARG0]]) : (f32) -> i1
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptosi.sat"(%[[ARG0]]) : (f32) -> i8
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptosi.sat"(%[[ARG0]]) : (f32) -> i16
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptosi.sat"(%[[ARG0]]) : (f32) -> i32
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptosi.sat"(%[[ARG0]]) : (f32) -> i64
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptoui.sat"(%[[ARG0]]) : (f32) -> i8
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptoui.sat"(%[[ARG0]]) : (f32) -> i16
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptoui.sat"(%[[ARG0]]) : (f32) -> i32
+// CHECK:         %{{.*}} = llvm.call_intrinsic "llvm.fptoui.sat"(%[[ARG0]]) : (f32) -> i64
 
 // -----
 
diff --git a/flang/test/Integration/fp-convert.f90 b/flang/test/Integration/fp-convert.f90
new file mode 100644
index 0000000000000..a042b28827b9a
--- /dev/null
+++ b/flang/test/Integration/fp-convert.f90
@@ -0,0 +1,236 @@
+! RUN: %flang -funsigned %s -o %t && %t | FileCheck %s
+! RUN: %flang -funsigned -emit-llvm -S -o - %s | FileCheck %s --check-prefix=LLVMIR
+
+module fp_convert_m
+  implicit none
+  interface set_and_print
+    module procedure set_and_print_r16
+    module procedure set_and_print_r8
+  end interface
+contains
+  subroutine set_and_print_r16(value)
+    real(kind=16), intent(in) :: value
+    integer(kind=1) :: i8
+    integer(kind=2) :: i16
+    integer(kind=4) :: i32
+    integer(kind=8) :: i64
+    integer(kind=16) :: i128
+    unsigned(kind=1) :: u8
+    unsigned(kind=2) :: u16
+    unsigned(kind=4) :: u32
+    unsigned(kind=8) :: u64
+    unsigned(kind=16) :: u128
+    print *, "Original real(16) value:", value
+    i8 = int(value, kind=1)
+    i16 = int(value, kind=2)
+    i32 = int(value, kind=4)
+    i64 = int(value, kind=8)
+    i128 = int(value, kind=16)
+    u8 = uint(value, kind=1)
+    u16 = uint(value, kind=2)
+    u32 = uint(value, kind=4)
+    u64 = uint(value, kind=8)
+    u128 = uint(value, kind=16)
+    print *, "Converted to 8-bit integer:", i8
+    print *, "Converted to 16-bit integer:", i16
+    print *, "Converted to 32-bit integer:", i32
+    print *, "Converted to 64-bit integer:", i64
+    print *, "Converted to 128-bit integer:", i128
+    print *, "Converted to 8-bit unsigned integer:", u8
+    print *, "Converted to 16-bit unsigned integer:", u16
+    print *, "Converted to 32-bit unsigned integer:", u32
+    print *, "Converted to 64-bit unsigned integer:", u64
+    print *, "Converted to 128-bit unsigned integer:", u128
+  end subroutine
+
+  subroutine set_and_print_r8(value)
+    real(kind=8), intent(in) :: value
+    integer(kind=1) :: i8
+    integer(kind=2) :: i16
+    integer(kind=4) :: i32
+    integer(kind=8) :: i64
+    integer(kind=16) :: i128
+    unsigned(kind=1) :: u8
+    unsigned(kind=2) :: u16
+    unsigned(kind=4) :: u32
+    unsigned(kind=8) :: u64
+    unsigned(kind=16) :: u128
+    print *, "Original real(8) value:", value
+    i8 = int(value, kind=1)
+    i16 = int(value, kind=2)
+    i32 = int(value, kind=4)
+    i64 = int(value, kind=8)
+    i128 = int(value, kind=16)
+    u8 = uint(value, kind=1)
+    u16 = uint(value, kind=2)
+    u32 = uint(value, kind=4)
+    u64 = uint(value, kind=8)
+    u128 = uint(value, kind=16)
+    print *, "Converted to 8-bit integer:", i8
+    print *, "Converted to 16-bit integer:", i16
+    print *, "Converted to 32-bit integer:", i32
+    print *, "Converted to 64-bit integer:", i64
+    print *, "Converted to 128-bit integer:", i128
+    print *, "Converted to 8-bit unsigned integer:", u8
+    print *, "Converted to 16-bit unsigned integer:", u16
+    print *, "Converted to 32-bit unsigned integer:", u32
+    print *, "Converted to 64-bit unsigned integer:", u64
+    print *, "Converted to 128-bit unsigned integer:", u128
+  end subroutine
+end module fp_convert_m
+
+program fp_convert
+  use ieee_arithmetic, only: ieee_value, ieee_quiet_nan, ieee_positive_inf, ieee_negative_inf
+  use fp_convert_m, only: set_and_print
+  implicit none
+
+  real(kind=8) :: nan, inf, ninf
+  nan = ieee_value(nan, ieee_quiet_nan)
+  inf = ieee_value(inf, ieee_positive_inf)
+  ninf = ieee_value(ninf, ieee_negative_inf)
+
+  call set_and_print(huge(0.0_8))
+  call set_and_print(-huge(0.0_8))
+  call set_and_print(huge(0.0_16))
+  call set_and_print(-huge(0.0_16))
+  call set_and_print(tiny(0.0_8))
+  call set_and_print(-tiny(0.0_8))
+  call set_and_print(tiny(0.0_16))
+  call set_and_print(-tiny(0.0_16))
+  call set_and_print(nan)
+  call set_and_print(inf)
+  call set_and_print(ninf)
+
+end program fp_convert
+
+! LLVMIR: call i8 @llvm.fptosi.sat.i8.f128(fp128 %{{.+}})
+! LLVMIR: call i16 @llvm.fptosi.sat.i16.f128(fp128 %{{.+}})
+! LLVMIR: call i32 @llvm.fptosi.sat.i32.f128(fp128 %{{.+}})
+! LLVMIR: call i64 @llvm.fptosi.sat.i64.f128(fp128 %{{.+}})
+! LLVMIR: call i128 @llvm.fptosi.sat.i128.f128(fp128 %{{.+}})
+! LLVMIR: call i8 @llvm.fptoui.sat.i8.f128(fp128 %{{.+}})
+! LLVMIR: call i16 @llvm.fptoui.sat.i16.f128(fp128 %{{.+}})
+! LLVMIR: call i32 @llvm.fptoui.sat.i32.f128(fp128 %{{.+}})
+! LLVMIR: call i64 @llvm.fptoui.sat.i64.f128(fp128 %{{.+}})
+! LLVMIR: call i128 @llvm.fptoui.sat.i128.f128(fp128 %{{.+}})
+! LLVMIR: call i8 @llvm.fptosi.sat.i8.f64(double %{{.+}})
+! LLVMIR: call i16 @llvm.fptosi.sat.i16.f64(double %{{.+}})
+! LLVMIR: call i32 @llvm.fptosi.sat.i32.f64(double %{{.+}})
+! LLVMIR: call i64 @llvm.fptosi.sat.i64.f64(double %{{.+}})
+! LLVMIR: call i128 @llvm.fptosi.sat.i128.f64(double %{{.+}})
+! LLVMIR: call i8 @llvm.fptoui.sat.i8.f64(double %{{.+}})
+! LLVMIR: call i16 @llvm.fptoui.sat.i16.f64(double %{{.+}})
+! LLVMIR: call i32 @llvm.fptoui.sat.i32.f64(double %{{.+}})
+! LLVMIR: call i64 @llvm.fptoui.sat.i64.f64(double %{{.+}})
+! LLVMIR: call i128 @llvm.fptoui.sat.i128.f64(double %{{.+}})
+
+! CHECK: Converted to 8-bit integer: 127
+! CHECK: Converted to 16-bit integer: 32767
+! CHECK: Converted to 32-bit integer: 2147483647
+! CHECK: Converted to 64-bit integer: 9223372036854775807
+! CHECK: Converted to 128-bit integer: 170141183460469231731687303715884105727
+! CHECK: Converted to 8-bit unsigned integer: 255
+! CHECK: Converted to 16-bit unsigned integer: 65535
+! CHECK: Converted to 32-bit unsigned integer: 4294967295
+! CHECK: Converted to 64-bit unsigned integer: 18446744073709551615
+! CHECK: Converted to 128-bit unsigned integer: 340282366920938463463374607431768211455
+! CHECK: Converted to 8-bit integer: -128
+! CHECK: Converted to 16-bit integer: -32768
+! CHECK: Converted to 32-bit integer: -2147483648
+! CHECK: Converted to 64-bit integer: -9223372036854775808
+! CHECK: Converted to 128-bit integer: -170141183460469231731687303715884105728
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 127
+! CHECK: Converted to 16-bit integer: 32767
+! CHECK: Converted to 32-bit integer: 2147483647
+! CHECK: Converted to 64-bit integer: 9223372036854775807
+! CHECK: Converted to 128-bit integer: 170141183460469231731687303715884105727
+! CHECK: Converted to 8-bit unsigned integer: 255
+! CHECK: Converted to 16-bit unsigned integer: 65535
+! CHECK: Converted to 32-bit unsigned integer: 4294967295
+! CHECK: Converted to 64-bit unsigned integer: 18446744073709551615
+! CHECK: Converted to 128-bit unsigned integer: 340282366920938463463374607431768211455
+! CHECK: Converted to 8-bit integer: -128
+! CHECK: Converted to 16-bit integer: -32768
+! CHECK: Converted to 32-bit integer: -2147483648
+! CHECK: Converted to 64-bit integer: -9223372036854775808
+! CHECK: Converted to 128-bit integer: -170141183460469231731687303715884105728
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 0
+! CHECK: Converted to 16-bit integer: 0
+! CHECK: Converted to 32-bit integer: 0
+! CHECK: Converted to 64-bit integer: 0
+! CHECK: Converted to 128-bit integer: 0
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 0
+! CHECK: Converted to 16-bit integer: 0
+! CHECK: Converted to 32-bit integer: 0
+! CHECK: Converted to 64-bit integer: 0
+! CHECK: Converted to 128-bit integer: 0
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 0
+! CHECK: Converted to 16-bit integer: 0
+! CHECK: Converted to 32-bit integer: 0
+! CHECK: Converted to 64-bit integer: 0
+! CHECK: Converted to 128-bit integer: 0
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 0
+! CHECK: Converted to 16-bit integer: 0
+! CHECK: Converted to 32-bit integer: 0
+! CHECK: Converted to 64-bit integer: 0
+! CHECK: Converted to 128-bit integer: 0
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 0
+! CHECK: Converted to 16-bit integer: 0
+! CHECK: Converted to 32-bit integer: 0
+! CHECK: Converted to 64-bit integer: 0
+! CHECK: Converted to 128-bit integer: 0
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0
+! CHECK: Converted to 8-bit integer: 127
+! CHECK: Converted to 16-bit integer: 32767
+! CHECK: Converted to 32-bit integer: 2147483647
+! CHECK: Converted to 64-bit integer: 9223372036854775807
+! CHECK: Converted to 128-bit integer: 170141183460469231731687303715884105727
+! CHECK: Converted to 8-bit unsigned integer: 255
+! CHECK: Converted to 16-bit unsigned integer: 65535
+! CHECK: Converted to 32-bit unsigned integer: 4294967295
+! CHECK: Converted to 64-bit unsigned integer: 18446744073709551615
+! CHECK: Converted to 128-bit unsigned integer: 340282366920938463463374607431768211455
+! CHECK: Converted to 8-bit integer: -128
+! CHECK: Converted to 16-bit integer: -32768
+! CHECK: Converted to 32-bit integer: -2147483648
+! CHECK: Converted to 64-bit integer: -9223372036854775808
+! CHECK: Converted to 128-bit integer: -170141183460469231731687303715884105728
+! CHECK: Converted to 8-bit unsigned integer: 0
+! CHECK: Converted to 16-bit unsigned integer: 0
+! CHECK: Converted to 32-bit unsigned integer: 0
+! CHECK: Converted to 64-bit unsigned integer: 0
+! CHECK: Converted to 128-bit unsigned integer: 0

@ashermancinelli
Copy link
Contributor Author

It looks like newer versions of gfortran actually have the same NaN -> int behavior that we will have if this patch is merged:

> gfortran --version           
GNU Fortran (GCC) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

> gfortran -O3 t.f90 && ./a.out
 -2147483648

> gfortran --version
GNU Fortran (GCC) 14.1.0
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

> gfortran -O3 t.f90 && ./a.out
           0

program main
  use ieee_arithmetic, only: ieee_value, ieee_quiet_nan
  implicit none
  real(kind=16)   :: f
  integer(kind=4) :: i
  f=ieee_value(f, ieee_quiet_nan)
  i=f
  print *, i
end program main

jeanPerier
jeanPerier requested changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Asher, aside from the test issues, looks good to me.
I added a few other reviewers working on other platforms for more visibility.

kiranchandramohan
kiranchandramohan reviewed Mar 11, 2025
View reviewed changes
flang/lib/Optimizer/CodeGen/CodeGen.cpp
Comment on lines +841 to +851
if (toFirTy.isUnsignedInteger()) {
auto intrinsicName =
mlir::StringAttr::get(convert.getContext(), "llvm.fptoui.sat");
rewriter.replaceOpWithNewOp<mlir::LLVM::CallIntrinsicOp>(
convert, toTy, intrinsicName, op0);
} else {
auto intrinsicName =
mlir::StringAttr::get(convert.getContext(), "llvm.fptosi.sat");
rewriter.replaceOpWithNewOp<mlir::LLVM::CallIntrinsicOp>(
convert, toTy, intrinsicName, op0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you know what these saturation intrinsics get lowered to? And is there a big difference in performance? Would it be possible to use the saturation intrinsic only when necessary? Or can that not be determined at compile time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They produce more instructions on x86 (when they cannot be const-folded away) (x86 godbolt link, more instructions, aarch64 godbolt link, both using fcvtzs), and if someone converted reals to integers in a hot loop they might see worse performance, however I was unable to find a difference in the performance tests that I ran. I'll be watching performance numbers after this is merged in case something comes up.

Would it be possible to use the saturation intrinsic only when necessary?

As long as we want the correct semantics for values only known at runtime, I don't think so. However, especially if performance issues come up, I think it would make sense to use the fptosi/fptoui instructions under some flag, maybe enabled by default above some optimization level. Do you think using the instructions instead of the saturated intrinsics under (for example) -ffast-math would be a good compromise if performance issues show up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think using the instructions instead of the saturated intrinsics under (for example) -ffast-math would be a good compromise if performance issues show up?

Personally, I agree with that approach. I think it is better to avoid having too many code generation paths unless there is an actual use case for it, in which case -ffast-math would sounds like the right flag to deviate from the requirements.

Please wait for @kiranchandramohan's feedback on the matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ashermancinelli for the reply. Just a few points, thinking out loud.

Would it be possible to use the saturation intrinsic only when necessary? Or can that not be determined at compile time?

This question I was asking here was about inferring from the real and integer types involved in the conversion. Like if we are converting from real(kind=2)/half-precision to integer(kind=4) then probably integer(kind=4) can hold all values without saturation.

gfortran (without fast-math) seems to be calling __fixtfsi.

There is also a question of whether vectorisation will work for these saturation intrinsics. I can see one issue filed against this topic by the rust community.
#59682

Do you think using the instructions instead of the saturated intrinsics under (for example) -ffast-math would be a good compromise if performance issues show up?

Personally, I agree with that approach. I think it is better to avoid having too many code generation paths unless there is an actual use case for it, in which case -ffast-math would sounds like the right flag to deviate from the requirements.

Please wait for @kiranchandramohan's feedback on the matter.

Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like if we are converting from real(kind=2)/half-precision to integer(kind=4) then probably integer(kind=4) can hold all values without saturation.

I see what you mean now and that seems like a great idea. I see you've approved this PR so I'll merge for now, but I would like to address this in a follow-up. Thanks!

@ashermancinelli
Remove end-to-end test case
42b9d64 
As Jean pointed out, this is better-suited to llvm-test-suite, so I'll
add it there after this change is merged.
@ashermancinelli
Copy link
Contributor Author

I'll add the end-to-end test to the llvm test suite once this is merged. Thanks for the reviews!

kiranchandramohan
kiranchandramohan approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

@ashermancinelli ashermancinelli merged commit 982527e into llvm:main Mar 12, 2025
11 checks passed
@ashermancinelli ashermancinelli mentioned this pull request Mar 13, 2025
Merged
frederik-h pushed a commit to frederik-h/llvm-project that referenced this pull request Mar 18, 2025
@ashermancinelli @frederik-h
[flang] Use saturated intrinsics for floating point to integer conver…
97727e6 
…sions (llvm#130686)

The saturated floating point conversion intrinsics match the semantics in the standard more closely than the fptosi/fptoui instructions.

Case 2 of 16.9.100 is

> INT (A [, KIND])
> If A is of type real, there are two cases: if |A| < 1, INT (A) has the
value 0; if |A| ≥ 1, INT (A) is the integer whose magnitude is the
largest integer that does not exceed the magnitude of A and whose sign
is the same as the sign of A.

Currently, converting a floating point value into an integer type too
small to hold the constant will be converted to poison in opt, leaving
us with garbage:

```
> cat t.f90
program main
  real(kind=16)   :: f
  integer(kind=4) :: i
  f=huge(f)
  i=f
  print *, i
end program main

# current upstream
> for i in `seq 10`; do; ./a.out; done
 -862156992
 -1497393344
 -739096768
 -1649494208
 1761228608
 -1959270592
 -746244288
 -1629194432
 -231217344
 382322496
```

With the saturated fptoui/fptosi intrinsics, we get the appropriate
values

```
# mine
> flang -O2 ./t.f90 && ./a.out
 2147483647

> perl -e 'printf "%d\n", (2 ** 31) - 1'
2147483647
```

One notable difference: NaNs being converted to ints will become zero, unlike current flang (and some other compilers). Newer versions of GCC have this behavior.
@SixWeining SixWeining mentioned this pull request May 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiranchandramohan kiranchandramohan kiranchandramohan approved these changes

@kkwli kkwli kkwli approved these changes

@jeanPerier jeanPerier jeanPerier approved these changes

@vzakhari vzakhari Awaiting requested review from vzakhari

Assignees
No one assigned
Labels
flang:codegen flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ashermancinelli @llvmbot @kiranchandramohan @kkwli @jeanPerier