[x86][SelectionDAG] Unroll vectorized FREM instructions which will be lowered to libcalls

[n-omer]

Summary

Currently, two element vectors produced as the result of a binary op are
widened to four element vectors on x86 by
DAGTypeLegalizer::WidenVecRes_BinaryCanTrap. If the op still isn't legal
after widening it is unrolled into scalar 4 scalar ops in selectionDAG before
being converted into a libcall. This way we end up with 4 libcalls (two of them
on known undef elements) instead of the original two libcalls.

This patch modifies DAGTypeLegalizer::WidenVectorResult to ensure
that if it is known that an binary op will be turned into a libcall, it is
unrolled instead of being widened. This prevents the creation of the extra
scalar instructions on known undef elements and (eventually) libcalls with
known undef parameters which would otherwise be created when the op gets
expanded post widening.

llvm/test/CodeGen/X86/frem-libcall.ll:

Regression test for the change.

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

The change in SelectionDAG as mentioned above.

[RKSimon]

Probably pull out the frem.ll and frem-libcall.ll tests into their own phab for review first - frem-libcall.ll in particular doesn't show the current problem in trunk (it generates 4 fmodf calls atm).

llvm/test/CodeGen/X86/frem-libcall.ll

5

Sort this to keep the description more easy to notice:

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=x86_64-linux-gnu < %s  | FileCheck %s

; Ensure vectorized FREMs are not widened/unrolled such that they get lowered
; into libcalls on undef elements.

llvm/test/CodeGen/X86/frem.ll

4 ↗

(On Diff #430716)

Sort this to keep the description more easy to notice:

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=x86_64-linux-gnu < %s  | FileCheck %s

; Basic test coverage for FREM

95 ↗

(On Diff #430716)

very pedantic - but maybe sort the vector tests by size - the v8f16/v4f32/v2f64 first - then the 256 / 512-bit variants.

309 ↗

(On Diff #430716)

frem_v4f32

[efriedma]

This makes the names and conventions here seem very confusing. The function is called WidenVecRes_BinaryCanTrap, but we're dealing with operations that can't actually trap. And it now has two different ways of unrolling an operation, depending on whether we're generating a libcall: the existing codepath uses smaller vectors, and the new one completely unrolls.

Can we unify the determination of whether we want to generate operations involving undef? And can we unify how we unroll an operation?

[craig.topper]

Why do we need to anything more than what we do for ISD::FSIN and friends already? LegalizeDAG.cpp doesn't have an Expand other than libcall for ISD::FREM. So checking TLI.isOperationExpand like we do for FSIN should be sufficient right?

[n-omer]

Hi @RKSimon, I've updated the tests for the current trunk and split them off into https://reviews.llvm.org/D126055.

[n-omer]

Updating the diff for completeness because D126055. Still looking into the reviewers comments.

[n-omer]

Update diff based on review comments.

[n-omer]

Add context to diff.

[n-omer]

Remove FIXME from test.

[RKSimon]

LGTM - its annoying that we can't reuse the similar code for the unary ops, but not a big issue. I think we will probably need to add FPOW and the integer division/remainder, but we can address that later.

@craig.topper @efriedma Any comments?

[spatel]

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

3698

Instead of copying this block, create a lambda above the switch and call it from here and the existing code?

auto unrollExpandedOp = [&]() {
  // We're going to widen this vector op to a legal type by padding with undef
  // elements. If the wide vector op is eventually going to be expanded to
  // scalar libcalls, then unroll into scalar ops now to avoid unnecessary
  // libcalls on the undef elements.
  EVT VT = N->getValueType(0);
  EVT WideVecVT = TLI.getTypeToTransformTo(*DAG.getContext(), VT);
  if (!TLI.isOperationLegalOrCustom(N->getOpcode(), WideVecVT) &&
      TLI.isOperationExpand(N->getOpcode(), VT.getScalarType())) {
    Res = DAG.UnrollVectorOp(N, WideVecVT.getVectorNumElements());
    return true;
  }
  return false;
};

[n-omer]

Update diff based on reviewer's comments.

Thanks @spatel.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

3703

remove these braces now?

3794–3795

these braces can go now?

[n-omer]

Update diff based on reviewer's comments.

Thanks @RKSimon.

[RKSimon]

LGTM

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

void DAGTypeLegalizer::WidenVectorResult(SDNode *N, unsigned ResNo) {
   LLVM_DEBUG(dbgs() << "Widen node result " << ResNo << ": "; N->dump(&DAG);
              dbgs() << "\n");
 
   // See if the target wants to custom widen this node.
   if (CustomWidenLowerNode(N, N->getValueType(ResNo)))
     return;
 
   SDValue Res = SDValue();
+
+  auto unrollExpandedOp = [&]() {
+    // We're going to widen this vector op to a legal type by padding with undef
+    // elements. If the wide vector op is eventually going to be expanded to
+    // scalar libcalls, then unroll into scalar ops now to avoid unnecessary
+    // libcalls on the undef elements.
+    EVT VT = N->getValueType(0);
+    EVT WideVecVT = TLI.getTypeToTransformTo(*DAG.getContext(), VT);
+    if (!TLI.isOperationLegalOrCustom(N->getOpcode(), WideVecVT) &&
+        TLI.isOperationExpand(N->getOpcode(), VT.getScalarType())) {
+      Res = DAG.UnrollVectorOp(N, WideVecVT.getVectorNumElements());
+      return true;
+    }
+    return false;
+  };
+
   switch (N->getOpcode()) {
   default:
 #ifndef NDEBUG
     dbgs() << "WidenVectorResult #" << ResNo << ": ";
     N->dump(&DAG);
     dbgs() << "\n";
 #endif
     llvm_unreachable("Do not know how to widen the result of this operator!");
#endif
   case ISD::VP_FADD:
   case ISD::VP_FSUB:
   case ISD::VP_FMUL:
   case ISD::VP_FDIV:
   case ISD::VP_FREM:
     Res = WidenVecRes_Binary(N);
     break;
 
+  case ISD::FREM:
+    if (unrollExpandedOp())
+      break;
+    // If the target has custom/legal support for the scalar FP intrinsic ops
+    // (they are probably not destined to become libcalls), then widen those
+    // like any other binary ops.
+    LLVM_FALLTHROUGH;
+
   case ISD::FADD:
   case ISD::FMUL:
   case ISD::FPOW:
   case ISD::FSUB:
   case ISD::FDIV:
-  case ISD::FREM:
   case ISD::SDIV:
   case ISD::UDIV:
   case ISD::SREM:
   case ISD::UREM:
     Res = WidenVecRes_BinaryCanTrap(N);
     break;
 
   case ISD::SMULFIX:
#include "llvm/IR/ConstrainedOps.def"
   case ISD::FLOG10:
   case ISD::FLOG2:
   case ISD::FNEARBYINT:
   case ISD::FRINT:
   case ISD::FROUND:
   case ISD::FROUNDEVEN:
   case ISD::FSIN:
   case ISD::FSQRT:
-  case ISD::FTRUNC: {
-    // We're going to widen this vector op to a legal type by padding with undef
-    // elements. If the wide vector op is eventually going to be expanded to
-    // scalar libcalls, then unroll into scalar ops now to avoid unnecessary
-    // libcalls on the undef elements.
-    EVT VT = N->getValueType(0);
-    EVT WideVecVT = TLI.getTypeToTransformTo(*DAG.getContext(), VT);
-    if (!TLI.isOperationLegalOrCustom(N->getOpcode(), WideVecVT) &&
-        TLI.isOperationExpand(N->getOpcode(), VT.getScalarType())) {
-      Res = DAG.UnrollVectorOp(N, WideVecVT.getVectorNumElements());
+  case ISD::FTRUNC:
+    if (unrollExpandedOp())
       break;
-    }
-  }
     // If the target has custom/legal support for the scalar FP intrinsic ops
-  // (they are probably not destined to become libcalls), then widen those like
-  // any other unary ops.
+    // (they are probably not destined to become libcalls), then widen those
+    // like any other unary ops.
     LLVM_FALLTHROUGH;
 
   case ISD::ABS:
   case ISD::BITREVERSE:
   case ISD::BSWAP:
   case ISD::CTLZ:
   case ISD::CTLZ_ZERO_UNDEF:
   case ISD::CTPOP:
   case ISD::CTTZ:

llvm/test/CodeGen/X86/frem-libcall.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=x86_64-linux-gnu < %s  | FileCheck %s
 
-; FIXME: Ensure vectorized FREMs are not widened/unrolled such that they get lowered
+; Ensure vectorized FREMs are not widened/unrolled such that they get lowered
 ; into libcalls on undef elements.
 
 define float @frem(<2 x float> %a0, <2 x float> %a1, <2 x float> %a2, <2 x float> *%p3) nounwind {
 ; CHECK-LABEL: frem:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    pushq %rbx
-; CHECK-NEXT:    subq $80, %rsp
+; CHECK-NEXT:    subq $64, %rsp
 ; CHECK-NEXT:    movq %rdi, %rbx
 ; CHECK-NEXT:    movaps %xmm2, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
 ; CHECK-NEXT:    movaps %xmm1, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
-; CHECK-NEXT:    movaps %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
-; CHECK-NEXT:    shufps {{.*#+}} xmm0 = xmm0[3,3,3,3]
-; CHECK-NEXT:    shufps {{.*#+}} xmm1 = xmm1[3,3,3,3]
-; CHECK-NEXT:    callq fmodf@PLT
-; CHECK-NEXT:    movaps %xmm0, (%rsp) # 16-byte Spill
-; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 16-byte Reload
-; CHECK-NEXT:    movhlps {{.*#+}} xmm0 = xmm0[1,1]
-; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm1 # 16-byte Reload
-; CHECK-NEXT:    movhlps {{.*#+}} xmm1 = xmm1[1,1]
-; CHECK-NEXT:    callq fmodf@PLT
-; CHECK-NEXT:    unpcklps (%rsp), %xmm0 # 16-byte Folded Reload
-; CHECK-NEXT:    # xmm0 = xmm0[0],mem[0],xmm0[1],mem[1]
 ; CHECK-NEXT:    movaps %xmm0, (%rsp) # 16-byte Spill
-; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 16-byte Reload
-; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm1 # 16-byte Reload
 ; CHECK-NEXT:    callq fmodf@PLT
 ; CHECK-NEXT:    movaps %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
-; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 16-byte Reload
+; CHECK-NEXT:    movaps (%rsp), %xmm0 # 16-byte Reload
 ; CHECK-NEXT:    shufps {{.*#+}} xmm0 = xmm0[1,1,1,1]
 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm1 # 16-byte Reload
 ; CHECK-NEXT:    shufps {{.*#+}} xmm1 = xmm1[1,1,1,1]
 ; CHECK-NEXT:    callq fmodf@PLT
 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm1 # 16-byte Reload
 ; CHECK-NEXT:    unpcklps {{.*#+}} xmm1 = xmm1[0],xmm0[0],xmm1[1],xmm0[1]
-; CHECK-NEXT:    unpcklpd (%rsp), %xmm1 # 16-byte Folded Reload
-; CHECK-NEXT:    # xmm1 = xmm1[0],mem[0]
 ; CHECK-NEXT:    divps {{[-0-9]+}}(%r{{[sb]}}p), %xmm1 # 16-byte Folded Reload
 ; CHECK-NEXT:    movaps %xmm1, %xmm0
 ; CHECK-NEXT:    shufps {{.*#+}} xmm0 = xmm0[1,1],xmm1[1,1]
 ; CHECK-NEXT:    addss %xmm1, %xmm0
 ; CHECK-NEXT:    movlps %xmm1, (%rbx)
-; CHECK-NEXT:    addq $80, %rsp
+; CHECK-NEXT:    addq $64, %rsp
 ; CHECK-NEXT:    popq %rbx
 ; CHECK-NEXT:    retq
   %frem = frem <2 x float> %a0, %a1
   %fdiv = fdiv <2 x float> %frem, %a2
   %ex0 = extractelement <2 x float> %fdiv, i32 0
   %ex1 = extractelement <2 x float> %fdiv, i32 1
   %res = fadd float %ex0, %ex1
   store <2 x float> %fdiv, <2 x float> *%p3
   ret float %res
 }