[PATCH] D71828: [InstCombine] Convert vector store to scalar store if only one element updated

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think this is safe now (although limited to a single basic block), but I don't have enough experience with memop transforms to confidently approve. Please get a 2nd look from @lebedev.ri @jdoerfert @efriedma or someone else.

I'm also not sure if there's a better way to preserve metadata. For example, we have "copyMetadataForLoad()" - is there something like that that we can use here?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1193
+
+  if (LoadInst *Load = dyn_cast<LoadInst>(Source)) {
+    Value *SrcAddr = Load->getPointerOperand()->stripPointerCasts();
----------------
Using 'auto *' is recommended with dyn_cast.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1213-1217
+    auto ElePtrType = NewElement->getType()->getPointerTo();
+    auto ElePtr =
+        IC.Builder.CreatePointerCast(SI.getPointerOperand(), ElePtrType);
+    auto GEP = IC.Builder.CreateInBoundsGEP(NewElement->getType(), ElePtr, Idx);
+    auto NSI = new StoreInst(NewElement, GEP);
----------------
Using 'auto'  on these lines is not recommended.
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/test/Transforms/InstCombine/load-insert-store.ll:7
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = load <16 x i8>, <16 x i8>* [[Q:%.*]], align 16
-; CHECK-NEXT:    [[VECINS:%.*]] = insertelement <16 x i8> [[TMP0]], i8 [[S:%.*]], i32 3
-; CHECK-NEXT:    store <16 x i8> [[VECINS]], <16 x i8>* [[Q]], align 16
+; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr <16 x i8>, <16 x i8>* [[Q:%.*]], i64 0, i64 3
+; CHECK-NEXT:    store i8 [[S:%.*]], i8* [[TMP0]], align 1
----------------
Need to regenerate the CHECK lines? All tests should have 'inbounds' on the GEP, right?


================
Comment at: llvm/test/Transforms/InstCombine/load-insert-store.ll:70
 
 define void @insert_store_mem_modify(<16 x i8>* %p, <16 x i8>* %q, <16 x i8>* noalias %r, i8 %s) {
 ; CHECK-LABEL: @insert_store_mem_modify(
----------------
Please add a test comment to explain the 2 transforms. Something like:
  ; p and q may alias, so it is not safe to remove the first load.
  ; r is known to not alias the other pointers, so it is safe to remove the second load. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71828