[PATCH] D138883: [SelectionDAG][PowerPC] Memset reuse vector element for tail store

[ChenZheng via Phabricator via llvm-commits]

shchenz accepted this revision as: shchenz.
shchenz added a comment.
This revision is now accepted and ready to land.

This LGTM with some nits.

Let's first target for the memset cases as this is the common case where splat values happens.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7644
     // If this store is smaller than the largest store see whether we can get
     // the smaller value for free with a truncate.
     SDValue Value = MemSetValue;
----------------
Nit: this comment needs update?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1634
+  if (auto *VTy = dyn_cast<VectorType>(VectorTy)) {
+    if (VTy->getScalarType()->isIntegerTy()) {
+      if (ElemSizeInBits == 32) {
----------------
nit: We may need comments here why we don't try to extract constant for `ElemSizeInBits` 8/16. (I guess the reason is we don't have benefit as we need `li` to load the index and this `li` can also be used to load the 8/16 bit imm?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17105
+        // be generated.
+        if (TailSize > 2 && TailSize <= 4) {
+          return MVT::v8i16;
----------------
tingwang wrote:
> shchenz wrote:
> > If using `stfd` is allowed for tail size 5/6/7, then can we use `stfd` for tail size 3/4 too? (I assume the change here impacts cases `memsetTailV1B3` and `memsetTailV1B4`?)
> It seems `TargetLowering::findOptimalMemOpLowering()` decides the type of each store. I guess if we change the type for the size 3/4 case from i32 to i64, then it will result in stfd.
Thanks. Better to add some comment here why we need to set the type to `MVT::v8i16`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138883