[CodeGen] Don't blindly combine (fp_round (fp_round x)) to (fp_round x).

[ab]

Summary

Hi all,

Double-rounding and single-rounding can have different results, but we have a DAGCombine that turns the former into the latter.

I believe that's unsafe in the general case, but we can still do it:

• if we know the first rounding is precise; or
• if unsafe FP math is enabled.

-Ahmed

[ab]

+llvm-commits

[ab]

.. and without the unused pointer in tests!

[mehdi_amini]

Good catch, the code looks good to me, but wait for someone else to sign off on it.

[t.p.northover]

Hi Ahmed,

The logic looks sound to me too. I think you should commit it.

Tim.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N0CFP)
     return DAG.getNode(ISD::FP_ROUND, SDLoc(N), VT, N0, N1);
 
   // fold (fp_round (fp_extend x)) -> x
   if (N0.getOpcode() == ISD::FP_EXTEND && VT == N0.getOperand(0).getValueType())
     return N0.getOperand(0);
 
   // fold (fp_round (fp_round x)) -> (fp_round x)
   if (N0.getOpcode() == ISD::FP_ROUND) {
-    // This is a value preserving truncation if both round's are.
-    bool IsTrunc = N->getConstantOperandVal(1) == 1 &&
-                   N0.getNode()->getConstantOperandVal(1) == 1;
+    const bool NIsTrunc = N->getConstantOperandVal(1) == 1;
+    const bool N0IsTrunc = N0.getNode()->getConstantOperandVal(1) == 1;
+    // If the first fp_round isn't a value preserving truncation, it might
+    // introduce a tie to the second fp_round, that wouldn't occur in the
+    // single-step fp_round we want to fold to.
+    // In other words, double rounding isn't the same as rounding.
+    // Also, this is a value preserving truncation iff both fp_round's are.
+    if (DAG.getTarget().Options.UnsafeFPMath || N0IsTrunc)
       return DAG.getNode(ISD::FP_ROUND, SDLoc(N), VT, N0.getOperand(0),
-                       DAG.getIntPtrConstant(IsTrunc));
+                         DAG.getIntPtrConstant(NIsTrunc && N0IsTrunc));
   }
 
   // fold (fp_round (copysign X, Y)) -> (copysign (fp_round X), Y)
   if (N0.getOpcode() == ISD::FCOPYSIGN && N0.getNode()->hasOneUse()) {
     SDValue Tmp = DAG.getNode(ISD::FP_ROUND, SDLoc(N0), VT,
                               N0.getOperand(0), N1);
     AddToWorklist(Tmp.getNode());
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,

test/CodeGen/AArch64/f16-convert.ll

; CHECK-NEXT: ret
 
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %val)
   store i16 %tmp, i16* %a, align 2
   ret void
 }
 
 define void @store1(i16* nocapture %a, double %val) nounwind {
 ; CHECK-LABEL: store1:
-; CHECK-NEXT: fcvt h0, d0
+; CHECK-NEXT: fcvt s0, d0
+; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str  h0, [x0]
 ; CHECK-NEXT: ret
 
   %conv = fptrunc double %val to float
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %conv)
   store i16 %tmp, i16* %a, align 2
   ret void
 }
 
 define void @store2(i16* nocapture %a, i32 %i, float %val) nounwind {
 ; CHECK-LABEL: store2:
 ; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str h0, [x0, w1, sxtw #1]
 ; CHECK-NEXT: ret
 
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %val)
   %idxprom = sext i32 %i to i64
   %arrayidx = getelementptr inbounds i16* %a, i64 %idxprom
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store3(i16* nocapture %a, i32 %i, double %val) nounwind {
 ; CHECK-LABEL: store3:
-; CHECK-NEXT: fcvt h0, d0
+; CHECK-NEXT: fcvt s0, d0
+; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str h0, [x0, w1, sxtw #1]
 ; CHECK-NEXT: ret
 
   %conv = fptrunc double %val to float
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %conv)
   %idxprom = sext i32 %i to i64
   %arrayidx = getelementptr inbounds i16* %a, i64 %idxprom
   store i16 %tmp, i16* %arrayidx, align 2
; CHECK-NEXT: ret
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %val)
   %arrayidx = getelementptr inbounds i16* %a, i64 %i
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store5(i16* nocapture %a, i64 %i, double %val) nounwind {
 ; CHECK-LABEL: store5:
-; CHECK-NEXT: fcvt h0, d0
+; CHECK-NEXT: fcvt s0, d0
+; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str h0, [x0, x1, lsl #1]
 ; CHECK-NEXT: ret
 
   %conv = fptrunc double %val to float
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %conv)
   %arrayidx = getelementptr inbounds i16* %a, i64 %i
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store6(i16* nocapture %a, float %val) nounwind {
 ; CHECK-LABEL: store6:
 ; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str h0, [x0, #20]
 ; CHECK-NEXT: ret
 
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %val)
   %arrayidx = getelementptr inbounds i16* %a, i64 10
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store7(i16* nocapture %a, double %val) nounwind {
 ; CHECK-LABEL: store7:
-; CHECK-NEXT: fcvt h0, d0
+; CHECK-NEXT: fcvt s0, d0
+; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: str h0, [x0, #20]
 ; CHECK-NEXT: ret
 
   %conv = fptrunc double %val to float
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %conv)
   %arrayidx = getelementptr inbounds i16* %a, i64 10
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store8(i16* nocapture %a, float %val) nounwind {
 ; CHECK-LABEL: store8:
 ; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: stur h0, [x0, #-20]
 ; CHECK-NEXT: ret
 
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %val)
   %arrayidx = getelementptr inbounds i16* %a, i64 -10
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 define void @store9(i16* nocapture %a, double %val) nounwind {
 ; CHECK-LABEL: store9:
-; CHECK-NEXT: fcvt h0, d0
+; CHECK-NEXT: fcvt s0, d0
+; CHECK-NEXT: fcvt h0, s0
 ; CHECK-NEXT: stur h0, [x0, #-20]
 ; CHECK-NEXT: ret
 
   %conv = fptrunc double %val to float
   %tmp = tail call i16 @llvm.convert.to.fp16.f32(float %conv)
   %arrayidx = getelementptr inbounds i16* %a, i64 -10
   store i16 %tmp, i16* %arrayidx, align 2
   ret void
 }
 
 declare i16 @llvm.convert.to.fp16.f32(float) nounwind readnone
 declare float @llvm.convert.from.fp16.f32(i16) nounwind readnone
 declare i16 @llvm.convert.to.fp16.f64(double) nounwind readnone
 declare double @llvm.convert.from.fp16.f64(i16) nounwind readnone

test/CodeGen/X86/fp-double-rounding.ll

+; RUN: llc < %s | FileCheck %s --check-prefix=CHECK --check-prefix=SAFE
+; RUN: llc < %s -enable-unsafe-fp-math | FileCheck %s --check-prefix=CHECK --check-prefix=UNSAFE
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64--"
+
+; CHECK-LABEL: double_rounding:
+; SAFE: callq __trunctfdf2
+; SAFE-NEXT: cvtsd2ss %xmm0
+; UNSAFE: callq __trunctfsf2
+; UNSAFE-NOT: cvt
+define void @double_rounding(fp128* %x, float* %f) {
+entry:
+  %0 = load fp128* %x, align 16
+  %1 = fptrunc fp128 %0 to double
+  %2 = fptrunc double %1 to float
+  store float %2, float* %f, align 4
+  ret void
+}
+
+; CHECK-LABEL: double_rounding_precise_first:
+; CHECK: fstps (%
+; CHECK-NOT: fstpl
+define void @double_rounding_precise_first(float* %f) {
+entry:
+  ; Hack, to generate a precise FP_ROUND to double
+  %precise = call double asm sideeffect "fld %st(0)", "={st(0)}"()
+  %0 = fptrunc double %precise to float
+  store float %0, float* %f, align 4
+  ret void
+}