[SystemZ] Avoid scalarization of S/UINT_TO_FP

[jonpa]

Summary

• Convert UINT_TO_FP DAG nodes with a wider result vector element type to first do a zero extend of the source vector:

The type legalizer will scalarize a 'v4f64 = uint_to_fp v4i16' as part of widening the illegally typed operand. By zero extending the source before type legalization, the node is instead split and kept as a vector operation.

Doing this before type legalization seemed like a simple solution that will handle any source vectors, such as v4i16, without having to worry about things like widening. The type legalizer seems to scalarize the uint_to_fp if the input/output vector type is not legal.

I thought about having a check here for a scalarized source operand (e.g. urem), in which case it might be less desired to first insert the source elements in order to do a vector convert. There was however no cases in benchmarks where also any user of the uint_to_fp was scalarized, so it seemed that this consideration can be ignored. This was a benchmark that first made me try this:

f470.lbm
cdlfbr         :                    4                    0       -4
vuplhh         :                    0                    2       +2
vuplhf         :                    0                    2       +2
vcdlgb         :                    0                    2       +2
vmrhg          :                   16                   14       -2
vlvgh          :                    0                    2       +2
vlvgb          :                    0                    2       +2
vuplhb         :                    0                    1       +1

The difference here was that a vector convert with a scalarized source, first vector elements are loaded and then unpacked. In that case it seems possibly better to do scalar conversions, since they support uint32 -> double and directly store into the right vector register: 2 x scalar conversions + 1 vmrhg. This however did not matter for performance and it was just one or two cases...

This is the check that was needed (and removed):

+bool SystemZTargetLowering::willBeScalarized(SDValue Op) const {
+  if (Op->isUndef())
+    return false;
+  if (Op->getOpcode() == ISD::VECTOR_SHUFFLE)
+    return willBeScalarized(Op->getOperand(0)) ||
+           willBeScalarized(Op->getOperand(1));
+
+  unsigned ScalarBits = Op.getValueType().getScalarSizeInBits();
+  MVT CheckVT = MVT::getVectorVT(MVT::getIntegerVT(ScalarBits),
+                                 SystemZ::VectorBits / ScalarBits);
+  return (getOperationAction(Op->getOpcode(), CheckVT) == Expand);
+}

 SDValue SystemZTargetLowering::combineUINT_TO_FP(
     SDNode *N, DAGCombinerInfo &DCI) const {

+  // Don't do this if Op is known to be scalarized.
+  if (willBeScalarized(Op))
+    return SDValue();
+

At first I used a check to only do this when the vector conversion was supported (v2f64 on z14 and also v4f32 on z15), but then I figured that it is still better to do the vector zero extend instead of extracting and then extending each element:

f510.parest_r
vlgvh          :                    0                   20      +20
vlgvf          :                  540                  520      -20
llhr           :                   90                  110      +20
vuplhh         :                   17                   12       -5

• Use a vllez + vle to load, shuffle and zero extend a vector with two elements in just two instructions:

I first tried to create the vllez with existing patterns by building a load and insert_vector_elt, but:

 t23: i32,ch = load<(load 4 from %ir.Src), anyext from i16> t0, t2, undef:i64
 t22: i32,ch = load<(load 4 from %ir.Src), anyext from i16> t0, t16, undef:i64
     t29: v8i16 = BUILD_VECTOR Constant:i32<0>...
            t31: v8i16 = insert_vector_elt t29, t23, Constant:i32<3>
          t32: v8i16 = insert_vector_elt t31, t22, Constant:i32<0>

=> canonicalization in DAGCombiner::visitINSERT_VECTOR_ELT() =>

      t29: v8i16 = BUILD_VECTOR Constant:i32<0>...
             t34: v8i16 = insert_vector_elt t29, t22, Constant:i32<0>
           t35: v8i16 = insert_vector_elt t34, t23, Constant:i32<3>

, which means that the insert-pattern for vllezh does not work.

Therefore a new SystemZISD::VLLEZ node seemed needed. The previous pattern for vllez has been renamed, and the new node SystemZISD::VLLEZ matches z_vllezi[8 - 64].

Tried also i32 -> i64, but this only changed one file (did not look like an improvement), and it involved extra searching code through a second VECTOR_SHUFFLE in tryVLLEZ(), so skipped.

Haven't tried v4i8 -> v4i32, since that would involve doing four loads instead of one, which seems a little worrisome.

I saw just a few cases on SPEC 06, where multiple LAYs were generated instead of one (13 bit displacement added twice, before both vllez and vle). It would have been better to do just one LAY and then use an immediate displacement in the vle. This seems like an existing problem which involves reg pressure consideration. Would it be worth trying a clean-up pass pre-RA that tried to minimize the number of LAYs? It could check if the base-address was killed at the first LAY and in that case reuse that LAYs result in the second one.

• Results

Only Imagick affected performance wise among benchmarks.

• combineUINT_TO_FP() gives 15% improvement
• tryVLLEZ() gives another 5-7% improvement.

Total improvement: 20-22%

imagick/morphology.s/MorphologyApply which is hot now uses 68 x (vllezh; vleh; vcdlgb).

(GCC does not do any vllez/vcdlgb, and nearly all uint to fp conversions are scalar, but it is still fast).

[uweigand]

Not yet looking at the implementation details, but a couple of comments on the overall approach:

• The UINT_TO_FP changes look good to me. The only question I have is whether we should do the same for SINT_TO_FP? (And in either case, this can probably be committed as a separate patch, apart from the VLLEZ changes.)

• I agree that using VLLEZ to implement the zero-extend is a good idea. I'm wondering about the implementation however; it seems odd to combine two quite different manners of emitting that instruction.

First of all, the "canonicalization in DAGCombiner::visitINSERT_VECTOR_ELT()" problem you mention seems to be a problem in general; the code with the two element-inserts could easily arise just from normal middle-end action (not just that new back-end code you added), and we really ought to be able to handle such code in general. So I think we should try to do that, either by overriding the DAGCombine with our own rule (if that's possible), or possibly handling this as a SystemZDAGToDAGISel::Select rule? In any case, I'm sceptical about the new DAG opcode -- it's always better to avoid those for semantics that *can* be described with standard opcodes, since custom opcodes are opaque to all further analysis by the middle-end ...

The other question is how to handle the zero-extend expansion. Logically, a vector zero-extend is fully equivalent to a shuffle intermixing bytes from the original vector with a zero vector. So I think the back-end ought to handle both cases in the same way. Now, we have logic that will intelligently handle shuffles (the whole GeneralShuffle logic). From what I can see, this never emits an UNPACK -- however, it should in theory be able to emit a MERGE, which would later (if one of the inputs is a zero vector) be transformed into an UNPACK via SystemZTargetLowering::combineMERGE.

Given that, maybe the optimal solution to catch all cases would be:

• first, expand vector zero-extend into a shuffle (always)
• second, make sure that the GeneralShuffle logic (and/or combineMERGE) detects the case where an optimal solution is via element inserts into a zero vector (and falls back to UNPACK otherwise)
• finally (as said above), make sure that element inserts into a zero vector get implemented via VLLEZ in all cases where this is possible

[jonpa]

• The UINT_TO_FP changes look good to me. The only question I have is whether we should do the same for SINT_TO_FP? (And in either case, this can probably be committed as a separate patch, apart from the VLLEZ changes.)

Yes, that looks also like a good idea. Patch updated to only do INT_TO_FP conversions - VLLEZ part will be posted separately.

[uweigand]

LGTM, thanks!

llvm/lib/Target/SystemZ/SystemZISelLowering.h

private:
   bool canLoadStoreByteSwapped(EVT VT) const;
   SDValue combineLOAD(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineSTORE(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineVECTOR_SHUFFLE(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineEXTRACT_VECTOR_ELT(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineJOIN_DWORDS(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineFP_ROUND(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineFP_EXTEND(SDNode *N, DAGCombinerInfo &DCI) const;
+  SDValue combineINT_TO_FP(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineBSWAP(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineBR_CCMASK(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineSELECT_CCMASK(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineGET_CCMASK(SDNode *N, DAGCombinerInfo &DCI) const;
   SDValue combineIntDIVREM(SDNode *N, DAGCombinerInfo &DCI) const;
 
   SDValue unwrapAddress(SDValue N) const override;
 

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

SystemZTargetLowering::SystemZTargetLowering(const TargetMachine &TM,
   setTargetDAGCombine(ISD::SIGN_EXTEND_INREG);
   setTargetDAGCombine(ISD::LOAD);
   setTargetDAGCombine(ISD::STORE);
   setTargetDAGCombine(ISD::VECTOR_SHUFFLE);
   setTargetDAGCombine(ISD::EXTRACT_VECTOR_ELT);
   setTargetDAGCombine(ISD::FP_ROUND);
   setTargetDAGCombine(ISD::STRICT_FP_ROUND);
   setTargetDAGCombine(ISD::FP_EXTEND);
+  setTargetDAGCombine(ISD::SINT_TO_FP);
+  setTargetDAGCombine(ISD::UINT_TO_FP);
   setTargetDAGCombine(ISD::STRICT_FP_EXTEND);
   setTargetDAGCombine(ISD::BSWAP);
   setTargetDAGCombine(ISD::SDIV);
   setTargetDAGCombine(ISD::UDIV);
   setTargetDAGCombine(ISD::SREM);
   setTargetDAGCombine(ISD::UREM);
 
   // Handle intrinsics.
for (auto *U : Vec->uses()) {
           return Extract0;
         }
       }
     }
   }
   return SDValue();
 }
 
+SDValue SystemZTargetLowering::combineINT_TO_FP(
+    SDNode *N, DAGCombinerInfo &DCI) const {
+  if (DCI.Level != BeforeLegalizeTypes)
+    return SDValue();
+  unsigned Opcode = N->getOpcode();
+  EVT OutVT = N->getValueType(0);
+  SelectionDAG &DAG = DCI.DAG;
+  SDValue Op = N->getOperand(0);
+  unsigned OutScalarBits = OutVT.getScalarSizeInBits();
+  unsigned InScalarBits = Op->getValueType(0).getScalarSizeInBits();
+
+  // Insert an extension before type-legalization to avoid scalarization, e.g.:
+  // v2f64 = uint_to_fp v2i16
+  // =>
+  // v2f64 = uint_to_fp (v2i64 zero_extend v2i16)
+  if (OutVT.isVector() && OutScalarBits > InScalarBits) {
+    MVT ExtVT = MVT::getVectorVT(MVT::getIntegerVT(OutVT.getScalarSizeInBits()),
+                                 OutVT.getVectorNumElements());
+    unsigned ExtOpcode =
+      (Opcode == ISD::UINT_TO_FP ? ISD::ZERO_EXTEND : ISD::SIGN_EXTEND);
+    SDValue ExtOp = DAG.getNode(ExtOpcode, SDLoc(N), ExtVT, Op);
+    return DAG.getNode(Opcode, SDLoc(N), OutVT, ExtOp);
+  }
+  return SDValue();
+}
+
 SDValue SystemZTargetLowering::combineBSWAP(
     SDNode *N, DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;
   // Combine BSWAP (LOAD) into LRVH/LRV/LRVG/VLBR
   if (ISD::isNON_EXTLoad(N->getOperand(0).getNode()) &&
       N->getOperand(0).hasOneUse() &&
       canLoadStoreByteSwapped(N->getValueType(0))) {
       SDValue Load = N->getOperand(0);
SDValue SystemZTargetLowering::PerformDAGCombine(SDNode *N,
   case ISD::STORE:              return combineSTORE(N, DCI);
   case ISD::VECTOR_SHUFFLE:     return combineVECTOR_SHUFFLE(N, DCI);
   case ISD::EXTRACT_VECTOR_ELT: return combineEXTRACT_VECTOR_ELT(N, DCI);
   case SystemZISD::JOIN_DWORDS: return combineJOIN_DWORDS(N, DCI);
   case ISD::STRICT_FP_ROUND:
   case ISD::FP_ROUND:           return combineFP_ROUND(N, DCI);
   case ISD::STRICT_FP_EXTEND:
   case ISD::FP_EXTEND:          return combineFP_EXTEND(N, DCI);
+  case ISD::SINT_TO_FP:
+  case ISD::UINT_TO_FP:         return combineINT_TO_FP(N, DCI);
   case ISD::BSWAP:              return combineBSWAP(N, DCI);
   case SystemZISD::BR_CCMASK:   return combineBR_CCMASK(N, DCI);
   case SystemZISD::SELECT_CCMASK: return combineSELECT_CCMASK(N, DCI);
   case SystemZISD::GET_CCMASK:  return combineGET_CCMASK(N, DCI);
   case ISD::SDIV:
   case ISD::UDIV:
   case ISD::SREM:
   case ISD::UREM:               return combineIntDIVREM(N, DCI);

llvm/test/CodeGen/SystemZ/vec-move-23.ll

+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z14 | FileCheck %s -check-prefixes=CHECK,Z14
+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z15 | FileCheck %s -check-prefixes=CHECK,Z15
+;
+; Check that int-to-fp conversions from a narrower type get a vector extension.
+
+define void @fun0(<2 x i8> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun0:
+; CHECK:      vuphb %v0, %v24
+; CHECK-NEXT: vuphh %v0, %v0
+; CHECK-NEXT: vuphf %v0, %v0
+; CHECK-NEXT: vcdgb %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = sitofp <2 x i8> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun1(<2 x i16> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun1:
+; CHECK:      vuphh %v0, %v24
+; CHECK-NEXT: vuphf %v0, %v0
+; CHECK-NEXT: vcdgb %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = sitofp <2 x i16> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun2(<2 x i32> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun2:
+; CHECK:      vuphf %v0, %v24
+; CHECK-NEXT: vcdgb %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = sitofp <2 x i32> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun3(<4 x i16> %Src, <4 x float>* %Dst) {
+; CHECK-LABEL: fun3:
+
+; Z14:      vuphh %v0, %v24
+; Z14-NEXT: vlgvf %r0, %v0, 3
+; Z14-NEXT: cefbr %f1, %r0
+; Z14-NEXT: vlgvf %r0, %v0, 2
+; Z14-NEXT: cefbr %f2, %r0
+; Z14-NEXT: vlgvf %r0, %v0, 1
+; Z14-NEXT: vmrhf %v1, %v2, %v1
+; Z14-NEXT: cefbr %f2, %r0
+; Z14-NEXT: vlgvf %r0, %v0, 0
+; Z14-NEXT: cefbr %f0, %r0
+; Z14-NEXT: vmrhf %v0, %v0, %v2
+; Z14-NEXT: vmrhg %v0, %v0, %v1
+; Z14-NEXT: vst %v0, 0(%r2), 3
+; Z14-NEXT: br  %r14
+
+; Z15:      vuphh %v0, %v24
+; Z15-NEXT: vcefb %v0, %v0, 0, 0
+; Z15-NEXT: vst         %v0, 0(%r2), 3
+; Z15-NEXT: br          %r14
+  %c = sitofp <4 x i16> %Src to <4 x float>
+  store <4 x float> %c, <4 x float>* %Dst
+  ret void
+}
+
+define void @fun4(<2 x i8> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun4:
+; CHECK:      vuplhb  %v0, %v24
+; CHECK-NEXT: vuplhh  %v0, %v0
+; CHECK-NEXT: vuplhf  %v0, %v0
+; CHECK-NEXT: vcdlgb  %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = uitofp <2 x i8> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun5(<2 x i16> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun5:
+; CHECK:      vuplhh  %v0, %v24
+; CHECK-NEXT: vuplhf  %v0, %v0
+; CHECK-NEXT: vcdlgb  %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = uitofp <2 x i16> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun6(<2 x i32> %Src, <2 x double>* %Dst) {
+; CHECK-LABEL: fun6:
+; CHECK:      vuplhf  %v0, %v24
+; CHECK-NEXT: vcdlgb  %v0, %v0, 0, 0
+; CHECK-NEXT: vst %v0, 0(%r2), 3
+; CHECK-NEXT: br  %r14
+  %c = uitofp <2 x i32> %Src to <2 x double>
+  store <2 x double> %c, <2 x double>* %Dst
+  ret void
+}
+
+define void @fun7(<4 x i16> %Src, <4 x float>* %Dst) {
+; CHECK-LABEL: fun7:
+
+; Z14:      vuplhh  %v0, %v24
+; Z14-NEXT: vlgvf %r0, %v0, 3
+; Z14-NEXT: celfbr  %f1, 0, %r0, 0
+; Z14-NEXT: vlgvf %r0, %v0, 2
+; Z14-NEXT: celfbr  %f2, 0, %r0, 0
+; Z14-NEXT: vlgvf %r0, %v0, 1
+; Z14-NEXT: vmrhf %v1, %v2, %v1
+; Z14-NEXT: celfbr  %f2, 0, %r0, 0
+; Z14-NEXT: vlgvf %r0, %v0, 0
+; Z14-NEXT: celfbr  %f0, 0, %r0, 0
+; Z14-NEXT: vmrhf %v0, %v0, %v2
+; Z14-NEXT: vmrhg %v0, %v0, %v1
+; Z14-NEXT: vst %v0, 0(%r2), 3
+; Z14-NEXT: br  %r14
+
+; Z15:      vuplhh  %v0, %v24
+; Z15-NEXT: vcelfb  %v0, %v0, 0, 0
+; Z15-NEXT: vst         %v0, 0(%r2), 3
+; Z15-NEXT: br          %r14
+  %c = uitofp <4 x i16> %Src to <4 x float>
+  store <4 x float> %c, <4 x float>* %Dst
+  ret void
+}
+