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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I put up a similar patch to handle `extractelement (load %ptr), %index` in a similar fashion: D100273 <https://reviews.llvm.org/D100273>. Together with this patch, they greatly improve codegen for certain code generated using the C/C++ matrix extension https://clang.godbolt.org/z/qsccPdPf4



================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:761
+                                 const MemoryLocation &Loc, AAResults &AA) {
+  for (BasicBlock::iterator BBI = Begin; BBI != End; ++BBI)
+    if (isModSet(AA.getModRefInfo(&*BBI, Loc)))
----------------
I think we need a limit here, to avoid excessive compile time?


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:802
+    Value *ElePtr =
+        Builder.CreatePointerCast(SI->getPointerOperand(), ElePtrType);
+    Value *GEP = Builder.CreateInBoundsGEP(NewElement->getType(), ElePtr, Idx);
----------------
Do we need a pointer cast here? I *think* we can just create a `GEP` with the vector pointer and indices `0, Idx`.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:805
+    StoreInst *NSI = Builder.CreateStore(NewElement, GEP);
+    NSI->copyMetadata(*SI, {LLVMContext::MD_nontemporal});
+    replaceValue(I, *NSI);
----------------
What about other metadata?


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:807
+    replaceValue(I, *NSI);
+    I.eraseFromParent();
+    return true;
----------------
Is there a reason to remove the instruction here? I don't think the other functions do so, so it might be better to keep things consistent (or change it for other patterns as well)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98240