[PATCH] D70223: [DAGCombine] Split vector load-update-store into single element stores

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

This is a very good idea. Love it!.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6640
+    else if (Current.getOpcode() == ISD::INSERT_VECTOR_ELT) {
+    // For INSERT_ELT nodes, we only have one path.
+      ++Depth;
----------------
Alignment seems strange. Please use clang-format


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6653
+      // (BUILD_VECTOR) operand in shuffle instr can't be what we want.
+      if (Op1.getOpcode() == ISD::BUILD_VECTOR) {
+        Path.push_back(std::make_pair(Current, 1));
----------------
What about one operand is undef? putting an undef into Path has no meaning?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6668
+      // And pick it from buffer.
+      while (!Path.empty()) {
+        std::pair<SDValue, unsigned> Top = Path.pop_back_val();
----------------
If Path is empty here, there is an infinite loop since Current is not changed. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6740
+
+        if (OriginalStates[i] != States[i])
+          Changed = true;
----------------
If changed is already set, don't need to set it again.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6754
+/// is updated, and split the store into multiple element stores.
+SDValue DAGCombiner::MatchVectorStoreSplit(StoreSDNode *N) {
+  SmallVector<std::pair<SDValue, unsigned>, 16> Buf;
----------------

I see for Buf, the second value is only given 0/1? Can we use a bool instead?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6758
+
+  // We don't support scalable vectors now.
+  EVT VecType = N->getValue().getValueType();
----------------
Move this comments down to line 6761. And also add other execluded cases in the comment


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6762
+
+  if (!N->isSimple() || VecType.isScalableVector() || !VecType.isVector())
+    return SDValue();
----------------
Can we handle indexed store too? I guess you also need to exclude that kind of store. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6784
+  // Go over the updated table to find updated elements.
+  SmallVector<int, 16> UpdatedElementsIdx;
+  for (unsigned i = 0; i < States.size(); ++i) {
----------------
I am thinking `UpdatedElementsIdx` and all the codes to update `UpdatedElementsIdx` is redundant. we could collect updated elements idx in `getVectorUpdates` and check whether States[i] need to update by its States[i].second?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6800
+  }
+
+  if (TLI.isCheapToSplitStore(N, UpdatedElementsIdx.size(), DAG)) {
----------------
Better to add an assert here that `UpdatedElementsIdx.size()` will never be larger than `VecLen`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1606
+
+  if (!VecType.isVector() || NumSplit >= VecType.getVectorNumElements())
+    return false;
----------------
Do we hava a perf testing shows that for <4 x i32> store, 3 scalar stores for i32 has a good perf?


================
Comment at: llvm/test/CodeGen/PowerPC/vector-store-split.ll:3
+; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mattr=+altivec < %s | FileCheck %s --check-prefixes=BE,CHECK
+
+; CHECK-LABEL: insert_store:
----------------
I think the test coverage is not enough. Especially the negative testing. For example `// We don't support shuffle which changes vector length.`, BUILD_VECTOR related testing, undef operand testing and so on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70223