[LegalizeDAG][Mips] Add an assert to protect a uint_to_fp implementation from double rounding. Add a i32->f32 uint_to_fp implementation that avoids this code.

[craig.topper]

Summary

The algorithm here only works if the sint_to_fp doesn't do any rounding. Otherwise it can round before the offset fixup is applied. Add an assert to protect this.

One test in tree was using this code for i32->f32 when f64 isn't legal. That test was only checking for not crashing until an earlier commit today. I've added a i32->f32 conversion using the same code as i64->f32 from TargetLowering. This is similar to what Mips gcc does for this config according to godbolt except they use control flow instead of selects. We should merge the new code with the code in TargetLowering, but the order of things is a little tricky right now. We call TLI first, then some code earlier in LegalizeDAG that tries to use i32->f32 by using i32->f64->f32. This new i32->f32 needs to be lower priority than i32->f64->f32. So I'm focusing on correctness first before refactoring this.

[uweigand]

Why do we need to copy this algorithm here? Can't you simply add this case to TargetLowering::expandUINT_TO_FP instead?

[uweigand]

In D72794#1822504, @uweigand wrote:

Why do we need to copy this algorithm here? Can't you simply add this case to TargetLowering::expandUINT_TO_FP instead?

Ah, nevermind -- I see you're talking about this in the summary ...

[craig.topper]

I was able to move the other version of this code from TargetLowering back to LegalizeDAG. So just need to add the new type to it.

[atanasyan]

LGTM

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

if (Node->isStrictFPOpcode()) {
       Result = DAG.getFPExtendOrRound(Sub, dl, DestVT);
     }
     return Result;
   }
   // Code below here assumes !isSigned without checking again.
   assert(!isSigned && "Legalize cannot Expand SINT_TO_FP for i64 yet");
 
   // TODO: Generalize this for use with other types.
-  if (SrcVT == MVT::i64 && DestVT == MVT::f32) {
-    LLVM_DEBUG(dbgs() << "Converting unsigned i64 to f32\n");
+  if ((SrcVT == MVT::i32 || SrcVT == MVT::i64) && DestVT == MVT::f32) {
+    LLVM_DEBUG(dbgs() << "Converting unsigned i32/i64 to f32\n");
     // For unsigned conversions, convert them to signed conversions using the
-    // algorithm from the x86_64 __floatundidf in compiler_rt.
+    // algorithm from the x86_64 __floatundisf in compiler_rt. That method
+    // should be valid for i32->f32 as well.
 
     // TODO: This really should be implemented using a branch rather than a
     // select.  We happen to get lucky and machinesink does the right
     // thing most of the time.  This would be a good candidate for a
     // pseudo-op, or, even better, for whole-function isel.
     EVT SetCCVT = getSetCCResultType(SrcVT);
 
     SDValue SignBitTest = DAG.getSetCC(
if (Node->isStrictFPOpcode()) {
       SDValue SignCvt = DAG.getNode(ISD::SINT_TO_FP, dl, DestVT, Or);
       Slow = DAG.getNode(ISD::FADD, dl, DestVT, SignCvt, SignCvt);
       Fast = DAG.getNode(ISD::SINT_TO_FP, dl, DestVT, Op0);
     }
 
     return DAG.getSelect(dl, DestVT, SignBitTest, Slow, Fast);
   }
 
-  // FIXME: This can produce slightly incorrect results. See details in
-  // FIXME: https://reviews.llvm.org/D69275
+  // The following optimization is valid only if every value in SrcVT (when
+  // treated as signed) is representable in DestVT.  Check that the mantissa
+  // size of DestVT is >= than the number of bits in SrcVT -1.
+  assert(APFloat::semanticsPrecision(DAG.EVTToAPFloatSemantics(DestVT)) >=
+             SrcVT.getSizeInBits() - 1 &&
+         "Cannot perform lossless SINT_TO_FP!");
+
   SDValue Tmp1;
   if (Node->isStrictFPOpcode()) {
     Tmp1 = DAG.getNode(ISD::STRICT_SINT_TO_FP, dl, { DestVT, MVT::Other },
                        { Node->getOperand(0), Op0 });
   } else
     Tmp1 = DAG.getNode(ISD::SINT_TO_FP, dl, DestVT, Op0);
 
   SDValue SignSet = DAG.getSetCC(dl, getSetCCResultType(SrcVT), Op0,

llvm/test/CodeGen/Mips/uitofp.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=mips -mattr=+single-float < %s | FileCheck %s
 
 define void @f0() nounwind {
 ; CHECK-LABEL: f0:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    addiu $sp, $sp, -8
 ; CHECK-NEXT:    addiu $1, $zero, 1
 ; CHECK-NEXT:    sw $1, 4($sp)
-; CHECK-NEXT:    lui $1, %hi($CPI0_0)
-; CHECK-NEXT:    addiu $1, $1, %lo($CPI0_0)
-; CHECK-NEXT:    lw $2, 4($sp)
-; CHECK-NEXT:    srl $3, $2, 29
-; CHECK-NEXT:    andi $3, $3, 4
-; CHECK-NEXT:    addu $1, $1, $3
-; CHECK-NEXT:    lwc1 $f0, 0($1)
-; CHECK-NEXT:    mtc1 $2, $f1
+; CHECK-NEXT:    lw $1, 4($sp)
+; CHECK-NEXT:    srl $2, $1, 1
+; CHECK-NEXT:    andi $3, $1, 1
+; CHECK-NEXT:    or $2, $3, $2
+; CHECK-NEXT:    mtc1 $2, $f0
+; CHECK-NEXT:    cvt.s.w $f0, $f0
+; CHECK-NEXT:    add.s $f0, $f0, $f0
+; CHECK-NEXT:    mtc1 $1, $f1
 ; CHECK-NEXT:    cvt.s.w $f1, $f1
-; CHECK-NEXT:    add.s $f0, $f1, $f0
-; CHECK-NEXT:    swc1 $f0, 0($sp)
+; CHECK-NEXT:    slti $1, $1, 0
+; CHECK-NEXT:    movn.s $f1, $f0, $1
+; CHECK-NEXT:    swc1 $f1, 0($sp)
 ; CHECK-NEXT:    jr $ra
 ; CHECK-NEXT:    addiu $sp, $sp, 8
 entry:
   %b = alloca i32, align 4
   %a = alloca float, align 4
   store volatile i32 1, i32* %b, align 4
   %0 = load volatile i32, i32* %b, align 4
   %conv = uitofp i32 %0 to float
   store float %conv, float* %a, align 4
   ret void
 }