[PATCH] D63624: [PowerPC] Exploit single instruction load-and-splat for word and doubleword

[Jinsong Ji via Phabricator via llvm-commits]

jsji requested changes to this revision.
jsji added a comment.
This revision now requires changes to proceed.

Some comments, but request changes as you will at least need to rebase to pick up non-relevant testcase update in https://reviews.llvm.org/rL365330.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1771
 bool PPC::isSplatShuffleMask(ShuffleVectorSDNode *N, unsigned EltSize) {
-  assert(N->getValueType(0) == MVT::v16i8 &&
-         (EltSize == 1 || EltSize == 2 || EltSize == 4));
+  assert(N->getValueType(0) == MVT::v16i8 && isPowerOf2_32(EltSize) &&
+         EltSize <= 8 && "Can only handle 1,2,4,8 byte element sizes");
----------------
Looks like you are repurposing this function: instead of using it just for 'VSPLT/VSPLTH/VSPLTW' , use it for `lxvdsx` as well.
I don't think it is a great idea to just update the assert here.

We should either rename the function, or re-structure the code to two different functions, in another NFC patch?



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8138
 
+bool sourceIsPlainLoad(SDValue Op, SDValue &InputLoad) {
+  if (Op.getOpcode() == ISD::BITCAST)
----------------
We can't support indexed load as well, and we are also trying to return the `InputLoad`, 
so maybe something like `getNormalLoadInput` and add comments about returning true for success?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8142
+  if (Op.getOpcode() == ISD::SCALAR_TO_VECTOR)
+    Op = Op.getOperand(0);
+  if (Op.getOpcode() == ISD::LOAD) {
----------------
Any other ISDs we can/should peek through? How about `ANY_EXTEND_VECTOR_INREG`/`EXTRACT_SUBVECTOR`? 



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8146
+    InputLoad = Op;
+    return !LD->isIndexed() && ISD::isNON_EXTLoad(LD);
+  }
----------------
Why not just `LD->isNormalLoad()`?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8278
+    if (DAG.isSplatValue(Op, true) &&
+        sourceIsPlainLoad(Op.getOperand(0), InputLoad)) {
+      LoadSDNode *LD = cast<LoadSDNode>(InputLoad);
----------------
Do we need to check `hasOneUse` here as well?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8793
+    bool IsFourByte = PPC::isSplatShuffleMask(SVOp, 4);
+    int SplatIdx = PPC::getVSPLTImmediate(SVOp, IsFourByte ? 4 : 8, DAG);
+
----------------
Similar to `isSplatShuffleMask`, we are repurposing `getVSPLTImmediate` as well, VSPLT* has only 3 forms (1/2/4), we should either rename this function, or use another wrapper.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:463
 
+      /// LD_SPLAT - a splatting load memory instruction (LXVDSX, LXVWSX).
+      LD_SPLAT,
----------------
This ISD has chain, please update the comments to describe it.


================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:62
+def SDT_PPCldsplat : SDTypeProfile<1, 1, [
+  SDTCisVec<0>, SDTCisSameAs<0, 1>
+]>;
----------------
Why `SDTCisSameAS<0,1>`? Shouldn't it be `SDTCisPtrTy<1>`?


================
Comment at: test/CodeGen/PowerPC/load-and-splat.ll:3
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr \
+; RUN:   -mtriple=powerpc64-unknown-unknown < %s | FileCheck %s \
+; RUN:   -check-prefix=P9
----------------
Any specific reason that we want to use `powerpc64` instead of `powerpc64le` for `pwr9`?


================
Comment at: test/CodeGen/PowerPC/power9-moves-and-splats.ll:15
 ; CHECK-NEXT:    blr
-
+;
 ; CHECK-BE-LABEL: test1:
----------------
nemanjai wrote:
> lebedev.ri wrote:
> > nemanjai wrote:
> > > Please forgive the trivial changes in this test case. The script that produces the checks apparently behaves slightly differently now and I would prefer to leave the test case exactly as produced by the script.
> > You can just regenerate all the affected files in a preparatory commit and rebase the patch.
> Ah, yeah. That's a good idea. I don't know why I didn't think of that. I'll definitely do that next time I see this issue.
I have committed https://reviews.llvm.org/rL365330 to include the new ';', you patch should auto-merge when you rebase.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63624