Don't combine fp_round (fp_round x) if f80 to f16 is generated

[pirama]

Summary

This patch skips DAG combine of fp_round (fp_round x) if it results in
an fp_round from f80 to f16.

fp_round from f80 to f16 always generates an expensive (and as yet,
unimplemented) libcall to __truncxfhf2. This prevents selection of
native f16 conversion instructions from f32 or f64. Moreover, the first
(value-preserving) fp_round from f80 to either f32 or f64 may become a
NOP in platforms like x86.

[ab]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9030 ↗

(On Diff #47869)

How do we know that the round is value-preserving?

[pirama]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9030 ↗

(On Diff #47869)

The first round being value-preserving is a pre-requisite for the folding (the if statement below). So, this patch doesn't change behavior if the first fp_round is not value-preserving.

[ab]

LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9030 ↗

(On Diff #47869)

Ah, you're right; I misread that, sorry.

test/CodeGen/X86/half.ll

270 ↗

(On Diff #47869)

What do you think of explicitly testing f80->f16 instead?

[pirama]

Thanks for the quick review :)

[srhines]

test/CodeGen/X86/half.ll

270 ↗

(On Diff #47869)

Pirama, I think you missed this question. :)

[pirama]

test/CodeGen/X86/half.ll

270 ↗

(On Diff #47869)

Aah, sorry, I missed this. I think you mean testing ftrunc from f80 -> say f32 followed by f32 -> f16. This doesn't trigger the folding because the rounds are not value preserving.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitFP_ROUND(SDNode *N) {
   // 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) {
     const bool NIsTrunc = N->getConstantOperandVal(1) == 1;
     const bool N0IsTrunc = N0.getNode()->getConstantOperandVal(1) == 1;
+
+    // Skip this folding if it results in an fp_round from f80 to f16.
+    //
+    // f80 to f16 always generates an expensive (and as yet, unimplemented)
+    // libcall to __truncxfhf2 instead of selecting native f16 conversion
+    // instructions from f32 or f64.  Moreover, the first (value-preserving)
+    // fp_round from f80 to either f32 or f64 may become a NOP in platforms like
+    // x86.
+    if (N0.getOperand(0).getValueType() == MVT::f80 && VT == MVT::f16)
+      return SDValue();
+
     // If the first fp_round isn't a value preserving truncation, it might
     // introduce a tie in 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) {
       SDLoc DL(N);
       return DAG.getNode(ISD::FP_ROUND, DL, VT, N0.getOperand(0),

llvm/trunk/test/CodeGen/X86/half.ll

 ; RUN: llc < %s -march=x86-64 -mtriple=x86_64-unknown-linux-gnu -mcpu=corei7 -mattr=-f16c -asm-verbose=false \
 ; RUN:   | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-LIBCALL
 ; RUN: llc < %s -march=x86-64 -mtriple=x86_64-unknown-linux-gnu -mcpu=corei7 -mattr=+f16c -asm-verbose=false \
 ; RUN:    | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-F16C
+; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -mattr +sse2 -asm-verbose=false \
+; RUN:    | FileCheck %s -check-prefix=CHECK-I686
 
 define void @test_load_store(half* %in, half* %out) {
 ; CHECK-LABEL: test_load_store:
 ; CHECK: movw (%rdi), [[TMP:%[a-z0-9]+]]
 ; CHECK: movw [[TMP]], (%rsi)
   %val = load half, half* %in
   store half %val, half* %out
   ret void

 ; CHECK: movw
 ; CHECK: movw
 ; CHECK: movw
   %v = fptrunc <4 x double> %a to <4 x half>
   store <4 x half> %v, <4 x half>* %p
   ret void
 }
 
+declare float @test_floatret();
+
+; On i686, if SSE2 is available, the return value from test_floatret is loaded
+; to f80 and then rounded to f32.  The DAG combiner should not combine this
+; fp_round and the subsequent fptrunc from float to half.
+define half @test_f80trunc_nodagcombine() #0 {
+; CHECK-LABEL: test_f80trunc_nodagcombine:
+; CHECK-I686-NOT: calll __truncxfhf2
+  %1 = call float @test_floatret()
+  %2 = fptrunc float %1 to half
+  ret half %2
+}
+
 attributes #0 = { nounwind }