[DAGCombine] fp_to_sint isSaturatingMinMax

[samparker]

Summary

Check for single smax pattern against zero when converting from a small enough float.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

5139

if (isNullOrNullSplat(N3)) ?

5146

Would it be useful to have a helper method instead of this raw semanticsMaxExponent call (+ increase for the sign bit?) - canEvaluateTruncated in InstCombineCasts has a similar calculation that could use the helper as well, but that already has a comment describing what's going on.

[samparker]

Thanks, extracted the bitwidth calculation into APFloatBase.

[RKSimon]

LGTM

llvm/include/llvm/ADT/APFloat.h

249

Maybe add a variable name IsSigned to the bool to make it more obvious?

[DavidSpickett]

I'm pretty sure this has caused a test suite failure on an SVE bot: https://lab.llvm.org/buildbot/#/builders/197/builds/3863

I'm just confirming the bisect now, but I see PerformMinMaxFpToSatCombine in the crash's backtrace.

fatal error: error in backend: Invalid size request on a scalable vector.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang -DNDEBUG -mcpu=a64fx -mllvm -scalable-vectorization=preferred -mllvm -treat-scalable-fixed-error-as-warning=false -O3 -w -fno-strict-aliasing -ffp-contract=off -DFMA_DISABLED=1 -ffp-contract=off -DFMA_DISABLED=1 -MD -MT MultiSource/Applications/oggenc/CMakeFiles/oggenc.dir/oggenc.c.o -MF CMakeFiles/oggenc.dir/oggenc.c.o.d -o CMakeFiles/oggenc.dir/oggenc.c.o -c /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/test/test-suite/MultiSource/Applications/oggenc/oggenc.c
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/test/test-suite/MultiSource/Applications/oggenc/oggenc.c'.
4.	Running pass 'AArch64 Instruction Selection' on function '@accumulate_fit'
 #0 0x0000aaaac079d8e0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x75a48e0)
 #1 0x0000aaaac079bb08 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x75a2b08)
 #2 0x0000aaaac079cd8c llvm::sys::CleanupOnSignal(unsigned long) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x75a3d8c)
 #3 0x0000aaaac070f468 (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) CrashRecoveryContext.cpp:0:0
 #4 0x0000aaaac070f3f8 llvm::CrashRecoveryContext::HandleExit(int) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x75163f8)
 #5 0x0000aaaac0798360 llvm::sys::Process::Exit(int, bool) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x759f360)
 #6 0x0000aaaabe42e830 llvm::BatchAAResults::~BatchAAResults() cc1_main.cpp:0:0
 #7 0x0000aaaac0717650 llvm::report_fatal_error(llvm::Twine const&, bool) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x751e650)
 #8 0x0000aaaac071751c (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x751e51c)
 #9 0x0000aaaac07568a0 llvm::TypeSize::operator unsigned long() const (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x755d8a0)
#10 0x0000aaaac07568c4 llvm::TypeSize::operator unsigned long() const (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x755d8c4)
#11 0x0000aaaac1748f8c PerformMinMaxFpToSatCombine(llvm::SDValue, llvm::SDValue, llvm::SDValue, llvm::SDValue, llvm::ISD::CondCode, llvm::SelectionDAG&) DAGCombiner.cpp:0:0
#12 0x0000aaaac17101dc (anonymous namespace)::DAGCombiner::visitIMINMAX(llvm::SDNode*) DAGCombiner.cpp:0:0
#13 0x0000aaaac1705e18 (anonymous namespace)::DAGCombiner::combine(llvm::SDNode*) DAGCombiner.cpp:0:0
#14 0x0000aaaac1702600 llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*, llvm::CodeGenOpt::Level) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x8509600)
#15 0x0000aaaac18596ac llvm::SelectionDAGISel::CodeGenAndEmitDAG() (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x86606ac)
#16 0x0000aaaac18583d0 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+0x865f3d0)
<...>

I'll get you the reproducer.

[DavidSpickett]

repro.tar.gz436 KBDownload

[samparker]

Thanks for the reproducer, I'll revert for now.

llvm/include/llvm/ADT/APFloat.h

enum IlogbErrorKinds {
     IEK_NaN = INT_MIN,
     IEK_Inf = INT_MAX
   };
 
   static unsigned int semanticsPrecision(const fltSemantics &);
   static ExponentType semanticsMinExponent(const fltSemantics &);
   static ExponentType semanticsMaxExponent(const fltSemantics &);
   static unsigned int semanticsSizeInBits(const fltSemantics &);
+  static unsigned int semanticsIntSizeInBits(const fltSemantics&,
+                                             bool isSigned);
 
   /// Returns the size of the floating point number (in bits) in the given
   /// semantics.
   static unsigned getSizeInBits(const fltSemantics &Sem);
 };
 
 namespace detail {
 

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 // Function to calculate whether the Min/Max pair of SDNodes (potentially
 // swapped around) make a signed saturate pattern, clamping to between a signed
 // saturate of -2^(BW-1) and 2^(BW-1)-1, or an unsigned saturate of 0 and 2^BW.
 // Returns the node being clamped and the bitwidth of the clamp in BW. Should
 // work with both SMIN/SMAX nodes and setcc/select combo. The operands are the
 // same as SimplifySelectCC. N0<N1 ? N2 : N3.
 static SDValue isSaturatingMinMax(SDValue N0, SDValue N1, SDValue N2,
                                   SDValue N3, ISD::CondCode CC, unsigned &BW,
-                                  bool &Unsigned) {
+                                  bool &Unsigned, SelectionDAG &DAG) {
   auto isSignedMinMax = [&](SDValue N0, SDValue N1, SDValue N2, SDValue N3,
                             ISD::CondCode CC) {
     // The compare and select operand should be the same or the select operands
     // should be truncated versions of the comparison.
     if (N0 != N2 && (N2.getOpcode() != ISD::TRUNCATE || N0 != N2.getOperand(0)))
       return 0;
     // The constants need to be the same or a truncated version of each other.
     ConstantSDNode *N1C = isConstOrConstSplat(N1);
     ConstantSDNode *N3C = isConstOrConstSplat(N3);
     if (!N1C || !N3C)
       return 0;
     const APInt &C1 = N1C->getAPIntValue();
     const APInt &C2 = N3C->getAPIntValue();
     if (C1.getBitWidth() < C2.getBitWidth() || C1 != C2.sext(C1.getBitWidth()))
       return 0;
     return CC == ISD::SETLT ? ISD::SMIN : (CC == ISD::SETGT ? ISD::SMAX : 0);
   };
 
   // Check the initial value is a SMIN/SMAX equivalent.
   unsigned Opcode0 = isSignedMinMax(N0, N1, N2, N3, CC);
   if (!Opcode0)
     return SDValue();
 
+  // We could only need one range check, if the fptosi could never produce
+  // the upper value.
+  if (N0.getOpcode() == ISD::FP_TO_SINT && Opcode0 == ISD::SMAX) {
+    if (isNullOrNullSplat(N3)) {
+      EVT IntVT = N0.getValueType();
+      EVT FPVT = N0.getOperand(0).getValueType();
+      if (FPVT.isSimple()) {
+        Type *InputTy = FPVT.getTypeForEVT(*DAG.getContext())->getScalarType();
+        const fltSemantics &Semantics = InputTy->getFltSemantics();
+        uint32_t MinBitWidth =
+          APFloatBase::semanticsIntSizeInBits(Semantics, /*isSigned*/ true);
+        if (IntVT.getSizeInBits() >= MinBitWidth) {
+          Unsigned = true;
+          BW = PowerOf2Ceil(MinBitWidth);
+          return N0;
+        }
+      }
+    }
+  }
+
   SDValue N00, N01, N02, N03;
   ISD::CondCode N0CC;
   switch (N0.getOpcode()) {
   case ISD::SMIN:
   case ISD::SMAX:
     N00 = N02 = N0.getOperand(0);
     N01 = N03 = N0.getOperand(1);
     N0CC = N0.getOpcode() == ISD::SMIN ? ISD::SETLT : ISD::SETGT;
static SDValue isSaturatingMinMax(SDValue N0, SDValue N1, SDValue N2,
   return SDValue();
 }
 
 static SDValue PerformMinMaxFpToSatCombine(SDValue N0, SDValue N1, SDValue N2,
                                            SDValue N3, ISD::CondCode CC,
                                            SelectionDAG &DAG) {
   unsigned BW;
   bool Unsigned;
-  SDValue Fp = isSaturatingMinMax(N0, N1, N2, N3, CC, BW, Unsigned);
+  SDValue Fp = isSaturatingMinMax(N0, N1, N2, N3, CC, BW, Unsigned, DAG);
   if (!Fp || Fp.getOpcode() != ISD::FP_TO_SINT)
     return SDValue();
   EVT FPVT = Fp.getOperand(0).getValueType();
   EVT NewVT = EVT::getIntegerVT(*DAG.getContext(), BW);
   if (FPVT.isVector())
     NewVT = EVT::getVectorVT(*DAG.getContext(), NewVT,
                              FPVT.getVectorElementCount());
   unsigned NewOpc = Unsigned ? ISD::FP_TO_UINT_SAT : ISD::FP_TO_SINT_SAT;

llvm/lib/Support/APFloat.cpp

namespace llvm {
   }
   APFloatBase::ExponentType
   APFloatBase::semanticsMinExponent(const fltSemantics &semantics) {
     return semantics.minExponent;
   }
   unsigned int APFloatBase::semanticsSizeInBits(const fltSemantics &semantics) {
     return semantics.sizeInBits;
   }
+  unsigned int APFloatBase::semanticsIntSizeInBits(const fltSemantics &semantics,
+                                                   bool isSigned) {
+    // The max FP value is pow(2, MaxExponent) * (1 + MaxFraction), so we need
+    // at least one more bit than the MaxExponent to hold the max FP value.
+    unsigned int MinBitWidth = semanticsMaxExponent(semantics) + 1;
+    // Extra sign bit needed.
+    if (isSigned)
+      ++MinBitWidth;
+    return MinBitWidth;
+  }
 
   unsigned APFloatBase::getSizeInBits(const fltSemantics &Sem) {
     return Sem.sizeInBits;
 }
 
 /* A bunch of private, handy routines.  */
 
 static inline Error createError(const Twine &Err) {

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

for (Value *IncValue : PN->incoming_values())
         return false;
     return true;
   }
   case Instruction::FPToUI:
   case Instruction::FPToSI: {
     // If the integer type can hold the max FP value, it is safe to cast
     // directly to that type. Otherwise, we may create poison via overflow
     // that did not exist in the original code.
-    //
-    // The max FP value is pow(2, MaxExponent) * (1 + MaxFraction), so we need
-    // at least one more bit than the MaxExponent to hold the max FP value.
     Type *InputTy = I->getOperand(0)->getType()->getScalarType();
     const fltSemantics &Semantics = InputTy->getFltSemantics();
-    uint32_t MinBitWidth = APFloatBase::semanticsMaxExponent(Semantics);
-    // Extra sign bit needed.
-    if (I->getOpcode() == Instruction::FPToSI)
-      ++MinBitWidth;
-    return Ty->getScalarSizeInBits() > MinBitWidth;
+    uint32_t MinBitWidth =
+      APFloatBase::semanticsIntSizeInBits(Semantics,
+          I->getOpcode() == Instruction::FPToSI);
+    return Ty->getScalarSizeInBits() >= MinBitWidth;
   }
   default:
     // TODO: Can handle more cases here.
     break;
   }
 
   return false;
 }

llvm/test/CodeGen/WebAssembly/fpclamptosat.ll

entry:
   %spec.store.select7 = select i1 %1, i64 %spec.store.select, i64 0
   %conv6 = trunc i64 %spec.store.select7 to i32
   ret i32 %conv6
 }
 
 define i32 @ustest_f16i32_cse(half %x) {
 ; CHECK-LABEL: ustest_f16i32_cse:
 ; CHECK:         .functype ustest_f16i32_cse (f32) -> (i32)
-; CHECK-NEXT:    .local i64
 ; CHECK-NEXT:  # %bb.0: # %entry
 ; CHECK-NEXT:    local.get 0
 ; CHECK-NEXT:    call __truncsfhf2
 ; CHECK-NEXT:    call __extendhfsf2
-; CHECK-NEXT:    i64.trunc_sat_f32_s
-; CHECK-NEXT:    local.tee 1
-; CHECK-NEXT:    i64.const 0
-; CHECK-NEXT:    local.get 1
-; CHECK-NEXT:    i64.const 0
-; CHECK-NEXT:    i64.gt_s
-; CHECK-NEXT:    i64.select
-; CHECK-NEXT:    i32.wrap_i64
+; CHECK-NEXT:    i32.trunc_sat_f32_u
 ; CHECK-NEXT:    # fallthrough-return
 entry:
   %conv = fptosi half %x to i64
   %0 = icmp sgt i64 %conv, 0
   %spec.store.select7 = select i1 %0, i64 %conv, i64 0
   %conv6 = trunc i64 %spec.store.select7 to i32
   ret i32 %conv6
 }
entry:
   %spec.store.select7 = select i1 %1, i32 %spec.store.select, i32 0
   %conv6 = trunc i32 %spec.store.select7 to i16
   ret i16 %conv6
 }
 
 define i16 @ustest_f16i16_cse(half %x) {
 ; CHECK-LABEL: ustest_f16i16_cse:
 ; CHECK:         .functype ustest_f16i16_cse (f32) -> (i32)
-; CHECK-NEXT:    .local i32
 ; CHECK-NEXT:  # %bb.0: # %entry
 ; CHECK-NEXT:    local.get 0
 ; CHECK-NEXT:    call __truncsfhf2
 ; CHECK-NEXT:    call __extendhfsf2
-; CHECK-NEXT:    i32.trunc_sat_f32_s
-; CHECK-NEXT:    local.tee 1
-; CHECK-NEXT:    i32.const 0
-; CHECK-NEXT:    local.get 1
-; CHECK-NEXT:    i32.const 0
-; CHECK-NEXT:    i32.gt_s
-; CHECK-NEXT:    i32.select
+; CHECK-NEXT:    i32.trunc_sat_f32_u
 ; CHECK-NEXT:    # fallthrough-return
 entry:
   %conv = fptosi half %x to i32
   %0 = icmp sgt i32 %conv, 0
   %spec.store.select7 = select i1 %0, i32 %conv, i32 0
   %conv6 = trunc i32 %spec.store.select7 to i16
   ret i16 %conv6
 }

 ; CHECK-NEXT:    # fallthrough-return
 entry:
   %conv = fptoui double %x to i128
   %spec.store.select = call i128 @llvm.umin.i128(i128 %conv, i128 18446744073709551616)
   %conv6 = trunc i128 %spec.store.select to i64
   ret i64 %conv6
 }
 
-
 define i64 @ustest_f64i64(double %x) {
 ; CHECK-LABEL: ustest_f64i64:
 ; CHECK:         .functype ustest_f64i64 (f64) -> (i64)
 ; CHECK-NEXT:    .local i32, i64, i64
 ; CHECK-NEXT:  # %bb.0: # %entry
 ; CHECK-NEXT:    global.get __stack_pointer
 ; CHECK-NEXT:    i32.const 16
 ; CHECK-NEXT:    i32.sub
entry:
   %spec.store.select7 = call i64 @llvm.smax.i64(i64 %spec.store.select, i64 0)
   %conv6 = trunc i64 %spec.store.select7 to i32
   ret i32 %conv6
 }
 
 define i32 @ustest_f16i32_mm_cse(half %x) {
 ; CHECK-LABEL: ustest_f16i32_mm_cse:
 ; CHECK:         .functype ustest_f16i32_mm_cse (f32) -> (i32)
-; CHECK-NEXT:    .local i64
 ; CHECK-NEXT:  # %bb.0: # %entry
 ; CHECK-NEXT:    local.get 0
 ; CHECK-NEXT:    call __truncsfhf2
 ; CHECK-NEXT:    call __extendhfsf2
-; CHECK-NEXT:    i64.trunc_sat_f32_s
-; CHECK-NEXT:    local.tee 1
-; CHECK-NEXT:    i64.const 0
-; CHECK-NEXT:    local.get 1
-; CHECK-NEXT:    i64.const 0
-; CHECK-NEXT:    i64.gt_s
-; CHECK-NEXT:    i64.select
-; CHECK-NEXT:    i32.wrap_i64
+; CHECK-NEXT:    i32.trunc_sat_f32_u
 ; CHECK-NEXT:    # fallthrough-return
 entry:
   %conv = fptosi half %x to i64
   %spec.store.select7 = call i64 @llvm.smax.i64(i64 %conv, i64 0)
   %conv6 = trunc i64 %spec.store.select7 to i32
   ret i32 %conv6
 }
 
entry:
   %spec.store.select7 = call i32 @llvm.smax.i32(i32 %spec.store.select, i32 0)
   %conv6 = trunc i32 %spec.store.select7 to i16
   ret i16 %conv6
 }
 
 define i16 @ustest_f16i16_mm_cse(half %x) {
 ; CHECK-LABEL: ustest_f16i16_mm_cse:
 ; CHECK:         .functype ustest_f16i16_mm_cse (f32) -> (i32)
-; CHECK-NEXT:    .local i32
 ; CHECK-NEXT:  # %bb.0: # %entry
 ; CHECK-NEXT:    local.get 0
 ; CHECK-NEXT:    call __truncsfhf2
 ; CHECK-NEXT:    call __extendhfsf2
-; CHECK-NEXT:    i32.trunc_sat_f32_s
-; CHECK-NEXT:    local.tee 1
-; CHECK-NEXT:    i32.const 0
-; CHECK-NEXT:    local.get 1
-; CHECK-NEXT:    i32.const 0
-; CHECK-NEXT:    i32.gt_s
-; CHECK-NEXT:    i32.select
+; CHECK-NEXT:    i32.trunc_sat_f32_u
 ; CHECK-NEXT:    # fallthrough-return
 entry:
   %conv = fptosi half %x to i32
   %spec.store.select7 = call i32 @llvm.smax.i32(i32 %conv, i32 0)
   %conv6 = trunc i32 %spec.store.select7 to i16
   ret i16 %conv6
 }