[PATCH] D98240: [VectorCombine] Simplify to scalar store if only one element updated

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:54
+static cl::opt<unsigned> IterationThreshold(
+    "vector-combine-iteration-threshold", cl::init(100), cl::Hidden,
+    cl::desc("Max number of instructions to scan for vector combining."));
----------------
This seems a bit high to start with, perhaps we should start with a smaller limit? Unfortunately there are not many instances of this pattern in the test-suite/SPEC2000/SPEC2006 so we can't really evaluate the impact.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:768
+  unsigned LoopCnt = 0;
+  for (BasicBlock::iterator BBI = Begin; BBI != End; ++BBI)
+    if (isModSet(AA.getModRefInfo(&*BBI, Loc)) ||
----------------
this potentially could be made a bit simpler by using `any_of` instead of the explicit loop.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:800
+    // Don't optimize for atomic/volatile load or stores.
+    if (!Load->isSimple() || Load->getParent() != SI->getParent() ||
+        SrcAddr != SI->getPointerOperand()->stripPointerCasts() ||
----------------
I think we are also missing test coverage for the case where load and store are not in the same BB?


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:801
+    if (!Load->isSimple() || Load->getParent() != SI->getParent() ||
+        SrcAddr != SI->getPointerOperand()->stripPointerCasts() ||
+        isMemModifiedBetween(Load->getIterator(), SI->getIterator(),
----------------
do we have test coverage for that?


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:802
+        SrcAddr != SI->getPointerOperand()->stripPointerCasts() ||
+        isMemModifiedBetween(Load->getIterator(), SI->getIterator(),
+                             MemoryLocation::get(SI), AA))
----------------
I think we still need some more coverage for the various scenarios for `isMemModifiedBetween`, .e.g. the case where we have stores in between that are must-alias, no-alias and may-alias. Also we should have a test for the limit.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:884
     auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-    VectorCombine Combiner(F, TTI, DT);
+    auto &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
+    VectorCombine Combiner(F, TTI, DT, AA);
----------------
I think there are some oddities with respect to `GlobalsAA` and it should also be preserved, e.g. see D82342 (same for the new PM)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98240