[PATCH] D106555: [PowerPC] handle more splat loads

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5825
+        // Perm = VPERM Load, Load, Mask
+        // Splat = VSPLTH 15/0, Perm
+        SDNode *Mask = CurDAG->getMachineNode(
----------------
VSPLTB?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5847
+        return;
+      } else if (N->getValueType(0) == MVT::v8i16) {
+        // v8i16 LD_SPLAT addr
----------------
clang-tidy has warneed, don't use 'else' after 'return'.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5855
+        // Splat = VSPLTH 7/0, Perm
+        SDNode *Mask = CurDAG->getMachineNode(
+            Subtarget->isLittleEndian() ? PPC::LVSR : PPC::LVSL, dl, MVT::v8i16,
----------------
The code patterns are very similar for these two cases, can we common them? 
eg: something like:

```

        SDNode *Mask = CurDAG->getMachineNode(
            Subtarget->isLittleEndian() ? PPC::LVSR : PPC::LVSL, dl, N->getValueType(0) ,
            CurDAG->getRegister(ZeroReg, MVT::i64), N->getOperand(1));

        SDNode *Load =
            CurDAG->getMachineNode(PPC::LVX, dl, N->getValueType(0), MVT::Other,
                                   {CurDAG->getRegister(ZeroReg, MVT::i64),
                                    N->getOperand(1), N->getOperand(0)});
             opcode = ...
          splatconst=...

             if (N->getValueType(0) == MVT::v8i16){
                 SDNode *LoadHigh = CurDAG->getMachineNode(
                                PPC::LVX, dl, MVT::v16i8, MVT::Other,
                                {SDValue(CurDAG->getMachineNode(
                               LIOpcode, dl, MVT::i32,
                               CurDAG->getTargetConstant(1, dl, MVT::i8)),
                               0),
                               N->getOperand(1), SDValue(Load, 1)});
           
             Load  = LoadHigh;
         opcode=...
        spltconst =...

         }

        CurDAG->ReplaceAllUsesOfValueWith(SDValue(N, 1), SDValue(Load, 1));
        transferMemOperands(N, Load);

        CurDAG->SelectNodeTo(
            N, opcode, N->getValueType(0),
            spliconst,
            SDValue(Perm, 0));

```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9206
+      // these cases.
+      if (NumUsesOfInputLD == 1 &&
+          ((Op->getValueType(0) == MVT::v2i64 &&
----------------
Can we split this into multiple early returns, with comments to corresponding ones.
```
    //case 1 ...
    if(...) 
       return SDValue();

    //case 2 ...
    if(...) 
       return SDValue();
    ...

```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2835
+def : Pat<(v4f32 (PPCldsplat ForceXForm:$A)),
+          (v4f32 (XXSPLTW (SUBREG_TO_REG (i64 1), (LFIWZX ForceXForm:$A), sub_64), 1))>;
 def : Pat<(v2i64 (PPCldsplat ForceXForm:$A)),
----------------
Do we need some conditional checking here? As `LFIWZX` is changing the type to int now, are there any precision change?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:3556
+def : Pat<(v8i16 (PPCldsplat ForceXForm:$A)),
+          (v8i16 (VSPLTHs 3, (MTVSRD (INSERT_SUBREG (i64 (IMPLICIT_DEF)), (LHZX ForceXForm:$A), sub_32))))>;
+def : Pat<(v16i8 (PPCldsplat ForceXForm:$A)),
----------------
Do we need to predicate 64 bit before using `MTVSRD`?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:4107
+def : Pat<(v8i16 (PPCldsplat ForceXForm:$A)),
+          (v8i16 (VSPLTHs 3, (LXSIHZX ForceXForm:$A)))>;
+def : Pat<(v16i8 (PPCldsplat ForceXForm:$A)),
----------------
Do we need alignment check here for `LXSIHZX`?


================
Comment at: llvm/test/CodeGen/PowerPC/canonical-merge-shuffles.ll:646
+; CHECK-P8-NEXT:    lfiwzx f0, r3, r4
+; CHECK-P8-NEXT:    xxspltd v2, f0, 0
+; CHECK-P8-NEXT:    vmrglb v2, v3, v2
----------------
`f0` here looks weird regarding register classes, `xxspltd` does need `vs0` to get the `element 0` of doubleword.  Maybe we should follow up to see why we are printing `f0` here in ASMPrinter.


================
Comment at: llvm/test/CodeGen/PowerPC/scalar_vector_test_3.ll:18
 ; P9LE-NEXT:    blr
-
+;
 ; P9BE-LABEL: s2v_test1:
----------------
Unrelated changes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106555/new/

https://reviews.llvm.org/D106555