[SelectionDAG] Improve support for promotion of <1 x fX> floating point argument types (PR31088)

[RKSimon]

Summary

PR31088 demonstrated that we were assuming that only integers require promotion from <1 x iX> types, when in fact float types may require it as well - in this case half floats.

This patch adds support for extension/truncation for both integer and float types.

[efriedma]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

368 ↗

(On Diff #96289)

Is it actually possible for this to truncate/round?

564 ↗

(On Diff #96289)

Unless I'm misreading this, we just set Val to an EXTRACT_VECTOR_ELT of type PartVT on the previous line; does this conversion do anything?

test/CodeGen/X86/pr31088.ll

14 ↗

(On Diff #96289)

This __gnu_f2h_ieee + __gnu_h2f_ieee sequence looks strange...

21 ↗

(On Diff #96289)

Only tangentially related to your patch, but I'm not sure I understand the lowering here; addss is not a half-precision operation (and therefore won't produce correctly rounded results).

[RKSimon]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

564 ↗

(On Diff #96289)

Good point - will remove this and add an assert to check that the Parts[0] has a PartVT value type.

test/CodeGen/X86/pr31088.ll

14 ↗

(On Diff #96289)

From what I can tell, half args always come in as single precision floats and we don't guarantee that the floats are only set to half precision, so we have to truncate them - and then as we're performing arithmetic they must be extended back again to singles for the fadd.

Interestingly we don't bother truncating+extending the single results - we leave it with excess precision on return.

[RKSimon]

Updated based on @efriedma comments

[RKSimon]

rebased against trunk

[RKSimon]

ping?

[efriedma]

(I don't understand how this is supposed to work, so I don't feel comfortable approving it.)

[spatel]

The title/summary shows the wrong PR#; it should be PR31088:
https://bugs.llvm.org/show_bug.cgi?id=31088

The patch itself seems mechanical in that it's repeating the integer fixups, but I wonder if we can get someone with half-FP experience to take a look too in case this isn't behaving how we expect on all targets? The suspicious x86 codegen is a separate issue as noted earlier, but that's better than crashing? :)

test/CodeGen/X86/pr31088.ll

2–3 ↗

(On Diff #96552)

Does having both SSE and AVX RUNs add value? Seems like the output is identical apart from 'v' prefixes, so I'd kill one of them just as a matter of saving test time.

[RKSimon]

test/CodeGen/X86/pr31088.ll

2–3 ↗

(On Diff #96552)

OK - I'll keep SSE. I'll add i686 targets instead as they will be coming from the stack instead of registers so will show another behaviour

[tra]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

532 ↗

(On Diff #96552)

Nit. I'd use VT.isFloatingPoint() and use FP_EXTEND only on types we positively know to be FP and leave behavior for all other types as it is right now. Otherwise you're making implicit assumption that all types are either integer or FP which, generally speaking, is not true. There's void, for instance.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

367 ↗

(On Diff #96552)

Same as above.

test/CodeGen/X86/pr31088.ll

2–3 ↗

(On Diff #96552)

It may be interesting to run the tests for NVPTX target as it supports both native FP16 and promote-to-fp32-calcualate->demote back to fp16 codegen modes.

[RKSimon]

Use VT.isFloatingPoint() to select float extension/truncation (and still use integer path by default).

Add NVPTX test as suggested.

[tra]

LGTM. The patch produces sensible code for NVPTX target with and without fp16 support in hardware.

llvm/trunk/include/llvm/CodeGen/SelectionDAG.h

#endif
   }
 
   /// \brief Returns an ISD::VECTOR_SHUFFLE node semantically equivalent to
   /// the shuffle node in input but with swapped operands.
   ///
   /// Example: shuffle A, B, <0,5,2,7> -> shuffle B, A, <4,1,6,3>
   SDValue getCommutedVectorShuffle(const ShuffleVectorSDNode &SV);
 
+  /// Convert Op, which must be of float type, to the
+  /// float type VT, by either extending or rounding (by truncation).
+  SDValue getFPExtendOrRound(SDValue Op, const SDLoc &DL, EVT VT);
+
   /// Convert Op, which must be of integer type, to the
   /// integer type VT, by either any-extending or truncating it.
   SDValue getAnyExtOrTrunc(SDValue Op, const SDLoc &DL, EVT VT);
 
   /// Convert Op, which must be of integer type, to the
   /// integer type VT, by either sign-extending or truncating it.
   SDValue getSExtOrTrunc(SDValue Op, const SDLoc &DL, EVT VT);
 

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

 /// The vectors to concatenate have length one - use a BUILD_VECTOR instead.
 SDValue DAGTypeLegalizer::ScalarizeVecOp_CONCAT_VECTORS(SDNode *N) {
   SmallVector<SDValue, 8> Ops(N->getNumOperands());
   for (unsigned i = 0, e = N->getNumOperands(); i < e; ++i)
     Ops[i] = GetScalarizedVector(N->getOperand(i));
   return DAG.getBuildVector(N->getValueType(0), SDLoc(N), Ops);
 }
 
-/// If the input is a vector that needs to be scalarized, it must be <1 x ty>,
-/// so just return the element, ignoring the index.
-SDValue DAGTypeLegalizer::ScalarizeVecOp_EXTRACT_VECTOR_ELT(SDNode *N) {
-  EVT VT = N->getValueType(0);
-  SDValue Res = GetScalarizedVector(N->getOperand(0));
-  if (Res.getValueType() != VT)
-    Res = DAG.getNode(ISD::ANY_EXTEND, SDLoc(N), VT, Res);
-  return Res;
-}
-
+/// If the input is a vector that needs to be scalarized, it must be <1 x ty>,
+/// so just return the element, ignoring the index.
+SDValue DAGTypeLegalizer::ScalarizeVecOp_EXTRACT_VECTOR_ELT(SDNode *N) {
+  EVT VT = N->getValueType(0);
+  SDValue Res = GetScalarizedVector(N->getOperand(0));
+  if (Res.getValueType() != VT)
+    Res = VT.isFloatingPoint()
+              ? DAG.getNode(ISD::FP_EXTEND, SDLoc(N), VT, Res)
+              : DAG.getNode(ISD::ANY_EXTEND, SDLoc(N), VT, Res);
+  return Res;
+}
 
 /// If the input condition is a vector that needs to be scalarized, it must be
 /// <1 x i1>, so just convert to a normal ISD::SELECT
 /// (still with vector output type since that was acceptable if we got here).
 SDValue DAGTypeLegalizer::ScalarizeVecOp_VSELECT(SDNode *N) {
   SDValue ScalarCond = GetScalarizedVector(N->getOperand(0));
   EVT VT = N->getValueType(0);
 

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

std::fill(ValueTypeNodes.begin(), ValueTypeNodes.end(),
             static_cast<SDNode*>(nullptr));
 
   EntryNode.UseList = nullptr;
   InsertNode(&EntryNode);
   Root = getEntryNode();
   DbgInfo->clear();
 }
 
+SDValue SelectionDAG::getFPExtendOrRound(SDValue Op, const SDLoc &DL, EVT VT) {
+  return VT.bitsGT(Op.getValueType())
+             ? getNode(ISD::FP_EXTEND, DL, VT, Op)
+             : getNode(ISD::FP_ROUND, DL, VT, Op, getIntPtrConstant(0, DL));
+}
+
 SDValue SelectionDAG::getAnyExtOrTrunc(SDValue Op, const SDLoc &DL, EVT VT) {
   return VT.bitsGT(Op.getValueType()) ?
     getNode(ISD::ANY_EXTEND, DL, VT, Op) :
     getNode(ISD::TRUNCATE, DL, VT, Op);
 }
 
 SDValue SelectionDAG::getSExtOrTrunc(SDValue Op, const SDLoc &DL, EVT VT) {
   return VT.bitsGT(Op.getValueType()) ?

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

static SDValue getCopyFromPartsVector(SelectionDAG &DAG, const SDLoc &DL,
   if (ValueVT.getVectorNumElements() != 1) {
     diagnosePossiblyInvalidConstraint(*DAG.getContext(), V,
                                       "non-trivial scalar-to-vector conversion");
     return DAG.getUNDEF(ValueVT);
   }
 
   EVT ValueSVT = ValueVT.getVectorElementType();
   if (ValueVT.getVectorNumElements() == 1 && ValueSVT != PartEVT)
-    Val = DAG.getAnyExtOrTrunc(Val, DL, ValueSVT);
+    Val = ValueVT.isFloatingPoint() ? DAG.getFPExtendOrRound(Val, DL, ValueSVT)
+                                    : DAG.getAnyExtOrTrunc(Val, DL, ValueSVT);
 
   return DAG.getBuildVector(ValueVT, DL, Val);
 }
 
 static void getCopyToPartsVector(SelectionDAG &DAG, const SDLoc &dl,
                                  SDValue Val, SDValue *Parts, unsigned NumParts,
                                  MVT PartVT, const Value *V);
 
if (PartEVT == ValueVT) {
       Val = DAG.getAnyExtOrTrunc(Val, DL, PartVT);
     } else{
       // Vector -> scalar conversion.
       assert(ValueVT.getVectorNumElements() == 1 &&
              "Only trivial vector-to-scalar conversions should get here!");
       Val = DAG.getNode(
           ISD::EXTRACT_VECTOR_ELT, DL, PartVT, Val,
           DAG.getConstant(0, DL, TLI.getVectorIdxTy(DAG.getDataLayout())));
-
-      Val = DAG.getAnyExtOrTrunc(Val, DL, PartVT);
     }
 
+    assert(Val.getValueType() == PartVT && "Unexpected vector part value type");
     Parts[0] = Val;
     return;
   }
 
   // Handle a multi-element vector.
   EVT IntermediateVT;
   MVT RegisterVT;
   unsigned NumIntermediates;

llvm/trunk/test/CodeGen/NVPTX/f16-instructions.ll

 ; CHECK-NOF16-NEXT: cvt.rn.f16.f32 [[R:%h[0-9]+]], [[R32]]
 ; CHECK-NEXT: st.param.b16    [func_retval0+0], [[R]];
 ; CHECK-NEXT: ret;
 define half @test_fadd(half %a, half %b) #0 {
   %r = fadd half %a, %b
   ret half %r
 }
 
+; CHECK-LABEL: test_fadd_v1f16(
+; CHECK-DAG:  ld.param.b16    [[A:%h[0-9]+]], [test_fadd_v1f16_param_0];
+; CHECK-DAG:  ld.param.b16    [[B:%h[0-9]+]], [test_fadd_v1f16_param_1];
+; CHECK-F16-NEXT:   add.rn.f16     [[R:%h[0-9]+]], [[A]], [[B]];
+; CHECK-NOF16-DAG:  cvt.f32.f16    [[A32:%f[0-9]+]], [[A]]
+; CHECK-NOF16-DAG:  cvt.f32.f16    [[B32:%f[0-9]+]], [[B]]
+; CHECK-NOF16-NEXT: add.rn.f32     [[R32:%f[0-9]+]], [[A32]], [[B32]];
+; CHECK-NOF16-NEXT: cvt.rn.f16.f32 [[R:%h[0-9]+]], [[R32]]
+; CHECK-NEXT: st.param.b16    [func_retval0+0], [[R]];
+; CHECK-NEXT: ret;
+define <1 x half> @test_fadd_v1f16(<1 x half> %a, <1 x half> %b) #0 {
+  %r = fadd <1 x half> %a, %b
+  ret <1 x half> %r
+}
+
 ; Check that we can lower fadd with immediate arguments.
 ; CHECK-LABEL: test_fadd_imm_0(
 ; CHECK-DAG:  ld.param.b16    [[B:%h[0-9]+]], [test_fadd_imm_0_param_0];
 ; CHECK-F16-DAG:    mov.b16        [[A:%h[0-9]+]], 0x3C00;
 ; CHECK-F16-NEXT:   add.rn.f16     [[R:%h[0-9]+]], [[B]], [[A]];
 ; CHECK-NOF16-DAG:  cvt.f32.f16    [[B32:%f[0-9]+]], [[B]]
 ; CHECK-NOF16-NEXT: add.rn.f32     [[R32:%f[0-9]+]], [[B32]], 0f3F800000;
 ; CHECK-NOF16-NEXT: cvt.rn.f16.f32 [[R:%h[0-9]+]], [[R32]]

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

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X86
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X64
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+f16c | FileCheck %s --check-prefix=F16C
 
+define <1 x half> @ir_fadd_v1f16(<1 x half> %arg0, <1 x half> %arg1) nounwind {
+; X86-LABEL: ir_fadd_v1f16:
+; X86:       # BB#0:
+; X86-NEXT:    subl $28, %esp
+; X86-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; X86-NEXT:    movss %xmm0, (%esp)
+; X86-NEXT:    calll __gnu_f2h_ieee
+; X86-NEXT:    movzwl %ax, %eax
+; X86-NEXT:    movl %eax, (%esp)
+; X86-NEXT:    calll __gnu_h2f_ieee
+; X86-NEXT:    fstpt {{[0-9]+}}(%esp) # 10-byte Folded Spill
+; X86-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; X86-NEXT:    movss %xmm0, (%esp)
+; X86-NEXT:    calll __gnu_f2h_ieee
+; X86-NEXT:    movzwl %ax, %eax
+; X86-NEXT:    movl %eax, (%esp)
+; X86-NEXT:    fldt {{[0-9]+}}(%esp) # 10-byte Folded Reload
+; X86-NEXT:    fstps {{[0-9]+}}(%esp)
+; X86-NEXT:    calll __gnu_h2f_ieee
+; X86-NEXT:    fstps {{[0-9]+}}(%esp)
+; X86-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; X86-NEXT:    addss {{[0-9]+}}(%esp), %xmm0
+; X86-NEXT:    movss %xmm0, {{[0-9]+}}(%esp)
+; X86-NEXT:    flds {{[0-9]+}}(%esp)
+; X86-NEXT:    addl $28, %esp
+; X86-NEXT:    retl
+;
+; X64-LABEL: ir_fadd_v1f16:
+; X64:       # BB#0:
+; X64-NEXT:    pushq %rax
+; X64-NEXT:    movss %xmm0, {{[0-9]+}}(%rsp) # 4-byte Spill
+; X64-NEXT:    movaps %xmm1, %xmm0
+; X64-NEXT:    callq __gnu_f2h_ieee
+; X64-NEXT:    movzwl %ax, %edi
+; X64-NEXT:    callq __gnu_h2f_ieee
+; X64-NEXT:    movss %xmm0, (%rsp) # 4-byte Spill
+; X64-NEXT:    movss {{[0-9]+}}(%rsp), %xmm0 # 4-byte Reload
+; X64-NEXT:    # xmm0 = mem[0],zero,zero,zero
+; X64-NEXT:    callq __gnu_f2h_ieee
+; X64-NEXT:    movzwl %ax, %edi
+; X64-NEXT:    callq __gnu_h2f_ieee
+; X64-NEXT:    addss (%rsp), %xmm0 # 4-byte Folded Reload
+; X64-NEXT:    popq %rax
+; X64-NEXT:    retq
+;
+; F16C-LABEL: ir_fadd_v1f16:
+; F16C:       # BB#0:
+; F16C-NEXT:    vcvtps2ph $4, %xmm1, %xmm1
+; F16C-NEXT:    vcvtph2ps %xmm1, %xmm1
+; F16C-NEXT:    vcvtps2ph $4, %xmm0, %xmm0
+; F16C-NEXT:    vcvtph2ps %xmm0, %xmm0
+; F16C-NEXT:    vaddss %xmm1, %xmm0, %xmm0
+; F16C-NEXT:    retq
+  %retval = fadd <1 x half> %arg0, %arg1
+  ret <1 x half> %retval
+}
+
 define <2 x half> @ir_fadd_v2f16(<2 x half> %arg0, <2 x half> %arg1) nounwind {
 ; X86-LABEL: ir_fadd_v2f16:
 ; X86:       # BB#0:
 ; X86-NEXT:    subl $64, %esp
 ; X86-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
 ; X86-NEXT:    movss %xmm0, (%esp)
 ; X86-NEXT:    calll __gnu_f2h_ieee
 ; X86-NEXT:    movzwl %ax, %eax