[PATCH] D71828: [InstCombine] Convert vector store to scalar store if only one element updated
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 6 09:02:45 PST 2020
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
More information about the llvm-commits
mailing list