[PowerPC] Replace the PPCISD:: SExtVElems with ISD::SIGN_EXTEND_INREG to leverage the combine rules

[steven.zhang]

Summary

The PPCISD::SExtVElems was added by commit https://reviews.llvm.org/D34009 However, we have another ISD node ISD::SIGN_EXTEND_INREG that perfectly match the semantics of SExtVElems. And the DAGCombiner has some combine rules for SIGN_EXTEND_INREG that produce better code.

[steven.zhang]

Gentle ping ...

[steven.zhang]

Gentle ping ...

[lkail]

Can you provide tests to demonstrate how ISD::SIGN_EXTEND_INREG works on PowerPC? Also, from my view, this patch should not be a NFC patch, since we somehow change execution paths, though we finally get the same codegen.

[steven.zhang]

Hmm, yes, not NFC as the dag combine rule will applied if with generic isd node. I will cook some tests.

[steven.zhang]

Rebase the patch. And I think the test case added by https://reviews.llvm.org/D34009 already cover my patch. We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

[steven.zhang]

Seems that the pre merge checker has problem in detecting the dependent patch landing or not. Remove the parent revision and trigger the check again.

[lkail]

We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

CMIIW, SExtVElems works on types that fit in a vector register, like v16i8. And according to codes SExtVElems generates, it intends to sext lower bits of target element type in-place, which suggests SIGN_EXTEND_INREG.

[steven.zhang]

In D70771#1880012, @lkail wrote:

We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

CMIIW, SExtVElems works on types that fit in a vector register, like v16i8. And according to codes SExtVElems generates, it intends to sext lower bits of target element type in-place, which suggests SIGN_EXTEND_INREG.

Yes, it is doing something like: a << N >> N; The SExtVElems has the same semantics with SIGN_EXTEND_INREG if its type is vector.

[lkail]

LGTM. Please wait a couple of days to land in case other reviewers have other concerns.

llvm/lib/Target/PowerPC/PPCISelLowering.h

enum NodeType : unsigned {
     /// Floating-point-to-interger conversion instructions
     FP_TO_UINT_IN_VSR,
     FP_TO_SINT_IN_VSR,
 
     /// VEXTS, ByteWidth - takes an input in VSFRC and produces an output in
     /// VSFRC that is sign-extended from ByteWidth to a 64-byte integer.
     VEXTS,
 
-    /// SExtVElems, takes an input vector of a smaller type and sign
-    /// extends to an output vector of a larger type.
-    SExtVElems,
-
     /// Reciprocal estimate instructions (unary FP ops).
     FRE,
     FRSQRTE,
 
     // VMADDFP, VNMSUBFP - The VMADDFP and VNMSUBFP instructions, taking
     // three v4f32 operands and producing a v4f32 result.
     VMADDFP,
     VNMSUBFP,

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

const char *PPCTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case PPCISD::VCMPo:           return "PPCISD::VCMPo";
   case PPCISD::LBRX:            return "PPCISD::LBRX";
   case PPCISD::STBRX:           return "PPCISD::STBRX";
   case PPCISD::LFIWAX:          return "PPCISD::LFIWAX";
   case PPCISD::LFIWZX:          return "PPCISD::LFIWZX";
   case PPCISD::LXSIZX:          return "PPCISD::LXSIZX";
   case PPCISD::STXSIX:          return "PPCISD::STXSIX";
   case PPCISD::VEXTS:           return "PPCISD::VEXTS";
-  case PPCISD::SExtVElems:      return "PPCISD::SExtVElems";
   case PPCISD::LXVD2X:          return "PPCISD::LXVD2X";
   case PPCISD::STXVD2X:         return "PPCISD::STXVD2X";
   case PPCISD::LOAD_VEC_BE:     return "PPCISD::LOAD_VEC_BE";
   case PPCISD::STORE_VEC_BE:    return "PPCISD::STORE_VEC_BE";
   case PPCISD::ST_VSR_SCAL_INT:
                                 return "PPCISD::ST_VSR_SCAL_INT";
   case PPCISD::COND_BRANCH:     return "PPCISD::COND_BRANCH";
   case PPCISD::BDNZ:            return "PPCISD::BDNZ";
for (int i = 0; i < SaveElts; i++)
       ShuffV[i * Stride] = i;
   else
     for (int i = 1; i <= SaveElts; i++)
       ShuffV[i * Stride - 1] = i - 1;
 
   SDValue ShuffleSrc2 =
       SignedConv ? DAG.getUNDEF(WideVT) : DAG.getConstant(0, dl, WideVT);
   SDValue Arrange = DAG.getVectorShuffle(WideVT, dl, Wide, ShuffleSrc2, ShuffV);
-  unsigned ExtendOp =
-      SignedConv ? (unsigned)PPCISD::SExtVElems : (unsigned)ISD::BITCAST;
 
   SDValue Extend;
-  if (!Subtarget.hasP9Altivec() && SignedConv) {
+  if (SignedConv) {
     Arrange = DAG.getBitcast(IntermediateVT, Arrange);
+    EVT ExtVT = Op.getOperand(0).getValueType();
+    if (Subtarget.hasP9Altivec())
+      ExtVT = EVT::getVectorVT(*DAG.getContext(), WideVT.getVectorElementType(),
+                               IntermediateVT.getVectorNumElements());
+
     Extend = DAG.getNode(ISD::SIGN_EXTEND_INREG, dl, IntermediateVT, Arrange,
-                         DAG.getValueType(Op.getOperand(0).getValueType()));
+                         DAG.getValueType(ExtVT));
   } else
-    Extend = DAG.getNode(ExtendOp, dl, IntermediateVT, Arrange);
+    Extend = DAG.getNode(ISD::BITCAST, dl, IntermediateVT, Arrange);
 
   return DAG.getNode(Opc, dl, Op.getValueType(), Extend);
 }
 
 SDValue PPCTargetLowering::LowerINT_TO_FP(SDValue Op,
                                           SelectionDAG &DAG) const {
   SDLoc dl(Op);
 
for (unsigned i = 0; i < N->getNumOperands(); i++) {
     CorrectElems = CorrectElems >> 8;
     Elems = Elems >> 8;
   }
 
   SDValue Shuffle =
       DAG.getVectorShuffle(Input.getValueType(), dl, Input,
                            DAG.getUNDEF(Input.getValueType()), ShuffleMask);
 
-  EVT Ty = N->getValueType(0);
-  SDValue BV = DAG.getNode(PPCISD::SExtVElems, dl, Ty, Shuffle);
-  return BV;
+  EVT VT = N->getValueType(0);
+  SDValue Conv = DAG.getBitcast(VT, Shuffle);
+
+  EVT ExtVT = EVT::getVectorVT(*DAG.getContext(),
+                               Input.getValueType().getVectorElementType(),
+                               VT.getVectorNumElements());
+  return DAG.getNode(ISD::SIGN_EXTEND_INREG, dl, VT, Conv,
+                     DAG.getValueType(ExtVT));
 }
 
 // Look for build vector patterns where input operands come from sign
 // extended vector_extract elements of specific indices. If the correct indices
-// aren't used, add a vector shuffle to fix up the indices and create a new
-// PPCISD:SExtVElems node which selects the vector sign extend instructions
+// aren't used, add a vector shuffle to fix up the indices and create
+// SIGN_EXTEND_INREG node which selects the vector sign extend instructions
 // during instruction selection.
 static SDValue combineBVOfVecSExt(SDNode *N, SelectionDAG &DAG) {
   // This array encodes the indices that the vector sign extend instructions
   // extract from when extending from one type to another for both BE and LE.
   // The right nibble of each byte corresponds to the LE incides.
   // and the left nibble of each byte corresponds to the BE incides.
   // For example: 0x3074B8FC  byte->word
   // For LE: the allowed indices are: 0x0,0x4,0x8,0xC

llvm/lib/Target/PowerPC/PPCInstrInfo.td

def SDT_PPCcv_fp_to_int  : SDTypeProfile<1, 1, [
   SDTCisFP<0>, SDTCisFP<1>
   ]>;
 def SDT_PPCstore_scal_int_from_vsr : SDTypeProfile<0, 3, [
   SDTCisVT<0, f64>, SDTCisPtrTy<1>, SDTCisPtrTy<2>
 ]>;
 def SDT_PPCVexts  : SDTypeProfile<1, 2, [
   SDTCisVT<0, f64>, SDTCisVT<1, f64>, SDTCisPtrTy<2>
 ]>;
-def SDT_PPCSExtVElems  : SDTypeProfile<1, 1, [
-  SDTCisVec<0>, SDTCisVec<1>
-]>;
 
 def SDT_PPCCallSeqStart : SDCallSeqStart<[ SDTCisVT<0, i32>,
                                            SDTCisVT<1, i32> ]>;
 def SDT_PPCCallSeqEnd   : SDCallSeqEnd<[ SDTCisVT<0, i32>,
                                          SDTCisVT<1, i32> ]>;
 def SDT_PPCvperm   : SDTypeProfile<1, 3, [
   SDTCisVT<3, v16i8>, SDTCisSameAs<0, 1>, SDTCisSameAs<0, 2>
 ]>;
def PPClfiwax : SDNode<"PPCISD::LFIWAX", SDT_PPClfiwx,
                        [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
 def PPClfiwzx : SDNode<"PPCISD::LFIWZX", SDT_PPClfiwx,
                        [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
 def PPClxsizx : SDNode<"PPCISD::LXSIZX", SDT_PPCLxsizx,
                        [SDNPHasChain, SDNPMayLoad]>;
 def PPCstxsix : SDNode<"PPCISD::STXSIX", SDT_PPCstxsix,
                        [SDNPHasChain, SDNPMayStore]>;
 def PPCVexts  : SDNode<"PPCISD::VEXTS", SDT_PPCVexts, []>;
-def PPCSExtVElems  : SDNode<"PPCISD::SExtVElems", SDT_PPCSExtVElems, []>;
 
 // Extract FPSCR (not modeled at the DAG level).
 def PPCmffs   : SDNode<"PPCISD::MFFS",
                        SDTypeProfile<1, 0, [SDTCisVT<0, f64>]>,
                        [SDNPHasChain]>;
 
 // Perform FADD in round-to-zero mode.
 def PPCfaddrtz: SDNode<"PPCISD::FADDRTZ", SDTFPBinOp, []>;

llvm/lib/Target/PowerPC/PPCInstrVSX.td

def : Pat<(v4i32 (build_vector HWordToWord.BE_A0, HWordToWord.BE_A1,
                       HWordToWord.BE_A2, HWordToWord.BE_A3)),
               (v4i32 (VEXTSH2W $A))>;
     def : Pat<(v4i32 (build_vector ByteToWord.BE_A0, ByteToWord.BE_A1,
                       ByteToWord.BE_A2, ByteToWord.BE_A3)),
               (v4i32 (VEXTSB2W $A))>;
     def : Pat<(v2i64 (build_vector ByteToDWord.BE_A0, ByteToDWord.BE_A1)),
               (v2i64 (VEXTSB2D $A))>;
   }
-
-  let Predicates = [HasP9Altivec] in {
-    def: Pat<(v2i64 (PPCSExtVElems v16i8:$A)),
-              (v2i64 (VEXTSB2D $A))>;
-    def: Pat<(v2i64 (PPCSExtVElems v8i16:$A)),
-              (v2i64 (VEXTSH2D $A))>;
-    def: Pat<(v2i64 (PPCSExtVElems v4i32:$A)),
-              (v2i64 (VEXTSW2D $A))>;
-    def: Pat<(v4i32 (PPCSExtVElems v16i8:$A)),
-              (v4i32 (VEXTSB2W $A))>;
-    def: Pat<(v4i32 (PPCSExtVElems v8i16:$A)),
-              (v4i32 (VEXTSH2W $A))>;
-  }
 }
 
 // Put this P9Altivec related definition here since it's possible to be 
 // selected to VSX instruction xvnegsp, avoid possible undef.
 let Predicates = [HasP9Altivec] in {
 
   def : Pat<(v4i32 (PPCvabsd v4i32:$A, v4i32:$B, (i32 0))),
             (v4i32 (VABSDUW $A, $B))>;