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

[qiucf]

Summary

This is vector-combine version of revision D71828, which simplifies load-insertelt-store pattern into getelementptr-store.

[qiucf]

Fix pipeline tests

[spatel]

Since the original proposal, MemorySSA has evolved. I still don't know much about that though. cc @asbirlea @nikic to see if this implementation is ok or should we use MemorySSA here?

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

777

Can shorten:

if (!SI || !SI->isSimple() ...)

829

This line of the comment is not accurate now. Remove or update.

[fhahn]

In D98240#2614181, @spatel wrote:

Since the original proposal, MemorySSA has evolved. I still don't know much about that though. cc @asbirlea @nikic to see if this implementation is ok or should we use MemorySSA here?

MemorySSA may be handy in this case, but it looks like it's not available close-by in the current pipeline position?

[nikic]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

810

This line shouldn't be dropped.

[qiucf]

Address comments

[fhahn]

I put up a similar patch to handle extractelement (load %ptr), %index in a similar fashion: 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

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

761

I think we need a limit here, to avoid excessive compile time?

802

Do we need a pointer cast here? I *think* we can just create a GEP with the vector pointer and indices 0, Idx.

805

What about other metadata?

807

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)

[qiucf]

Address comments

[qiucf]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

807

I think that's because we're operating on a store, which will not be erased automatically for empty use.

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

789

Why only allow constants here? If the index is a non-constant, it should be even more profitable, because most targets probably do not have instructions to insert at a variable index.

[qiucf]

Allow non-constant index.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

789

Good catch. Thanks.

[spatel]

LGTM, but @fhahn likely has a wider/fresher view of the expected patterns, so see if there are any other comments.

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

54

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.

768

this potentially could be made a bit simpler by using any_of instead of the explicit loop.

800

I think we are also missing test coverage for the case where load and store are not in the same BB?

801

do we have test coverage for that?

802

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.

884

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)

[lebedev.ri]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

53–54

Option name doesn't match the description/usage.
Iteration, to me, means how many times we will rerun this whole pass,
while you use it as a number of instructions to scan.

[qiucf]

Address comments.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

884

Do you mean dropping this and use result from GlobalsAA? I see GlobalsAA is already preserved.

[RKSimon]

llvm/test/Transforms/VectorCombine/X86/load-insert-store.ll

3

You've made this a X86 test but used a very generic data layout (and tested on big endian as well) - maybe move out of the x86 sub directory?

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

784

I'm not sure if the vector GEP will properly handle types that are non-power-of-2 properly. Perhaps it might be good to limit this to types for which the following holds? SI->typeSizeEqualsStoreSize(LI->getType())?

884

Ah never mind, I missed that it was already preserved.

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

809

I think we need to set the alignment here. For example, the original store could have a alignment less than the default for the type.

llvm/test/Transforms/InstCombine/load-insert-store.ll

1

I'm not sure why this has been removed?

llvm/test/Transforms/VectorCombine/X86/load-insert-store.ll

15

can you also add a test with element types > i8?

[qiucf]

Address some comments.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

784

Hmm.. Not sure what you mean, maybe something like

%0 = getelementptr inbounds <16 x i24>, <16 x i24>* %q, i32 0, i32 3
store i24 %s, i24* %0, align 4

this should be expected result. Or do we need to ensure scalar type size of load equals to store's value type size?

llvm/test/Transforms/InstCombine/load-insert-store.ll

1

It's moved from InstCombine to VectorCombine, not deleted.

llvm/test/Transforms/VectorCombine/X86/load-insert-store.ll

15

Do you mean insertelement <16 x i8> %0, i32 %s, i32 2? If not, there's a test for <8 x i16> below.

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

784

Yes this was what I meant. The created pointer looks OK, but I think this is an area where it's worth to be extra careful. I am not too familiar with the potential problems, but I think @bjope has experience with such targets. @bjope WDYT, do you think this would work as expected?

(my preference would be to start with dis-allowing the transform if the sizes don't match initially and enable it once we got confirmation that it is safe)

809

Perhaps I missed it, but could you add a test that specifies a smaller alignment for a store (!align 1) for a vector of i16 or larger?

llvm/test/Transforms/InstCombine/load-insert-store.ll

1

Oh right, so when it was originally added the plan was to optimize the pattern in instcombine? Might be worth moving the file in a separate commit and then just have the diff here show the changes by this patch,.

llvm/test/Transforms/VectorCombine/X86/load-insert-store.ll

15

yep I missed that one, sorry.

[bjope]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

784

Yes, I think this would be a bit more complicated if trying to support types for with DL.typeSizeEqualsStoreSize(SI->getType()) isn't true. Since the vectors are bit-packed you can't simply address a single vector element using a GEP otherwise. You wouldn't end up clobbering adjacent elements when doing the store (unless doing some kind of read-modify-write operation).

Maybe you even need to look at the typeAllocSize (comparing it with the storeSize) if alignment is important (to avoid misaligned stores).

But I also wonder if using GEP:s to address individual vector elements is something we do elsewhere. I know that https://llvm.org/docs/GetElementPtr.html#can-gep-index-into-vector-elements still says that "In the future, it will probably be outright disallowed.". Has there been discussions somewhere about opening up "pandoras box" (?) and start doing such things? If so, is that well tested somewhere?

For example something like this compiles without any complaints:

; RUN: llc -O3 -mtriple x86_64-- -o - %s
target datalayout = "E"
define void @foo(<8 x i4>* %q, i4 %s) {
  %p = getelementptr inbounds <8 x i4>, <8 x i4>* %q, i32 0, i32 3
  store i4 %s, i4* %p
  ret void
}

but it ends up writing to 3(%rdi) while the third element in that vector actually is at half of the byte at 1(%rdi).

[fhahn]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

784

Yes, I think this would be a bit more complicated if trying to support types for with DL.typeSizeEqualsStoreSize(SI->getType()) isn't true. Since the vectors are bit-packed you can't simply address a single vector element using a GEP otherwise. You wouldn't end up clobbering adjacent elements when doing the store (unless doing some kind of read-modify-write operation).

Thanks for confirming!

Maybe you even need to look at the typeAllocSize (comparing it with the storeSize) if alignment is important (to avoid misaligned stores).

We are setting the alignment of the store to the minimum of the alignment for the scalar store and the original alignment of the store. Would the be missing something?

But I also wonder if using GEP:s to address individual vector elements is something we do elsewhere.

It looks like at least instcombine likes to introduce such GEPs. Originally I was using something to the code in the link, which instcombine turned in a vector GEP, hence I updated D100273 to directly emit vector GEPS and suggested that here as well.

https://godbolt.org/z/q4o6fM3eP

[qiucf]

Update tests and store size constraint. Thanks for explanation from @bjope and @fhahn

[fhahn]

In D98240#2723175, @qiucf wrote:

Update tests and store size constraint. Thanks for explanation from @bjope and @fhahn

Thanks for the update! I think the only thing missing is a few tests for un-common vector types, like <8 x i4> or <4 x i31>. Probably worth to have at least 2 different tests with different types.

[qiucf]

Add more tests.

[fhahn]

LGTM, thanks!

[hgreving]

For targets not supporting scalar load from vector memory (like ours), this breaks it:

%43 = load <8 x i32>, <8 x i32> addrspace(201)* %1, align 32, !tbaa !28
%44 = extractelement <8 x i32> %43, i32 0

Now:

%43 = getelementptr inbounds <8 x i32>, <8 x i32> addrspace(201)* %1, i32 0, i32 0
%44 = load i32, i32 addrspace(201)* %43, align 32

Are targets expected to provide patterns?

[fhahn]

In D98240#2781048, @hgreving wrote:

For targets not supporting scalar load from vector memory (like ours), this breaks it:

%43 = load <8 x i32>, <8 x i32> addrspace(201)* %1, align 32, !tbaa !28
%44 = extractelement <8 x i32> %43, i32 0

Now:

%43 = getelementptr inbounds <8 x i32>, <8 x i32> addrspace(201)* %1, i32 0, i32 0
%44 = load i32, i32 addrspace(201)* %43, align 32

Are targets expected to provide patterns?

Interesting! I guess the code assumes that a scalar load is always possible & at least as cheap as the vector version. But I think it would make sense to ask the cost-model if that's the case. Not sure if it would be possible to test this with an in-tree target?

[lebedev.ri]

Hmm wait, i completely ignored this patch :/
Does this really not do any cost modelling?
This should at least check that scalar load isn't more costly
than the original vector load + insertelement + vector store.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

762–764

Does this ignore debuginfo?

[lebedev.ri]

Sorry for completely ignoring this :(
I'm fine with fixing this up as a followup, if that happens today.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

804–807

I'm certain this is a miscompile.
The alignment that was valid for a vector store
is not guaranteed to be valid for the store of a single vector element,
unless it's the 0'th element of course.

I think this needs to be something like

newalign = 1;
if(auto*C = dyn_cast<ConstantInt>(Idx)) {
  newalign = max(old store align, old load align);
  newalign = commonAlignment(newalign, Idx * DL.getTypeSize(NewElement.getType()))
}

[lebedev.ri]

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

802

Technically, size of the insertelement index doesn't have to match the size of the GEP index,
the latter is controlled in datalayout. I'm not sure if/how we need to deal with that here, however.

[hgreving]

In D98240#2781262, @fhahn wrote:

In D98240#2781048, @hgreving wrote:

For targets not supporting scalar load from vector memory (like ours), this breaks it:

%43 = load <8 x i32>, <8 x i32> addrspace(201)* %1, align 32, !tbaa !28
%44 = extractelement <8 x i32> %43, i32 0

Now:

%43 = getelementptr inbounds <8 x i32>, <8 x i32> addrspace(201)* %1, i32 0, i32 0
%44 = load i32, i32 addrspace(201)* %43, align 32

Are targets expected to provide patterns?

Interesting! I guess the code assumes that a scalar load is always possible & at least as cheap as the vector version. But I think it would make sense to ask the cost-model if that's the case. Not sure if it would be possible to test this with an in-tree target?

Hi thanks for getting back to me. I'm not sure if it's a cost model question, a straight-up disable switch for not morphing vector derefs into scalar might be better? Is there anything else in this pass that might do that? Unfortunately yes, I think there's no proper upstream target with this constraint. Though I am guessing I am not the only downstream target with a vector memory like that. The problem with trying to make this work is that I am worried about what happens to the pointer. Will I always be able to rely on that it will be aligned, probably not...

[fhahn]

In D98240#2782010, @hgreving wrote:

Interesting! I guess the code assumes that a scalar load is always possible & at least as cheap as the vector version. But I think it would make sense to ask the cost-model if that's the case. Not sure if it would be possible to test this with an in-tree target?

Hi thanks for getting back to me. I'm not sure if it's a cost model question, a straight-up disable switch for not morphing vector derefs into scalar might be better? Is there anything else in this pass that might do that? Unfortunately yes, I think there's no proper upstream target with this constraint. Though I am guessing I am not the only downstream target with a vector memory like that. The problem with trying to make this work is that I am worried about what happens to the pointer. Will I always be able to rely on that it will be aligned, probably not...

I guess it depends on whether the backend can legalize/convert back to a vector load. In general, relying on the middle-end to no scalarize those loads for correctness seems a bit fragile. I'm not sure if it makes sense to add a TTI hook that's not used by any in tree targets.

@qiucf would you be able to look into extending the code to check for the cost?

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

804–807

That's a good point. @qiucf can you look into this?

[qiucf]

In D98240#2788629, @fhahn wrote:

In D98240#2782010, @hgreving wrote:

Interesting! I guess the code assumes that a scalar load is always possible & at least as cheap as the vector version. But I think it would make sense to ask the cost-model if that's the case. Not sure if it would be possible to test this with an in-tree target?

Hi thanks for getting back to me. I'm not sure if it's a cost model question, a straight-up disable switch for not morphing vector derefs into scalar might be better? Is there anything else in this pass that might do that? Unfortunately yes, I think there's no proper upstream target with this constraint. Though I am guessing I am not the only downstream target with a vector memory like that. The problem with trying to make this work is that I am worried about what happens to the pointer. Will I always be able to rely on that it will be aligned, probably not...

I guess it depends on whether the backend can legalize/convert back to a vector load. In general, relying on the middle-end to no scalarize those loads for correctness seems a bit fragile. I'm not sure if it makes sense to add a TTI hook that's not used by any in tree targets.

@qiucf would you be able to look into extending the code to check for the cost?

I'll look into it, thanks. Sorry for the late notice.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

 static cl::opt<bool> DisableVectorCombine(
     "disable-vector-combine", cl::init(false), cl::Hidden,
     cl::desc("Disable all vector combine transforms"));
 
 static cl::opt<bool> DisableBinopExtractShuffle(
     "disable-binop-extract-shuffle", cl::init(false), cl::Hidden,
     cl::desc("Disable binop extract to shuffle transforms"));
 
 static const unsigned InvalidIndex = std::numeric_limits<unsigned>::max();
 
 namespace {
 class VectorCombine {
 public:
   VectorCombine(Function &F, const TargetTransformInfo &TTI,
-                const DominatorTree &DT)
-      : F(F), Builder(F.getContext()), TTI(TTI), DT(DT) {}
+                const DominatorTree &DT, AAResults &AA)
+      : F(F), Builder(F.getContext()), TTI(TTI), DT(DT), AA(AA) {}
 
   bool run();
 
 private:
   Function &F;
   IRBuilder<> Builder;
   const TargetTransformInfo &TTI;
   const DominatorTree &DT;
+  AAResults &AA;
 
   bool vectorizeLoadInsert(Instruction &I);
   ExtractElementInst *getShuffleExtract(ExtractElementInst *Ext0,
                                         ExtractElementInst *Ext1,
                                         unsigned PreferredExtractIndex) const;
   bool isExtractExtractCheap(ExtractElementInst *Ext0, ExtractElementInst *Ext1,
                              unsigned Opcode,
                              ExtractElementInst *&ConvertToShuffle,
                              unsigned PreferredExtractIndex);
   void foldExtExtCmp(ExtractElementInst *Ext0, ExtractElementInst *Ext1,
                      Instruction &I);
   void foldExtExtBinop(ExtractElementInst *Ext0, ExtractElementInst *Ext1,
                        Instruction &I);
   bool foldExtractExtract(Instruction &I);
   bool foldBitcastShuf(Instruction &I);
   bool scalarizeBinopOrCmp(Instruction &I);
   bool foldExtractedCmps(Instruction &I);
+  bool foldSingleElementStore(Instruction &I);
 };
 } // namespace
 
 static void replaceValue(Value &Old, Value &New) {
   Old.replaceAllUsesWith(&New);
   New.takeName(&Old);
 }
 
bool VectorCombine::foldExtractedCmps(Instruction &I) {
   Value *VecLogic = Builder.CreateBinOp(cast<BinaryOperator>(I).getOpcode(),
                                         VCmp, Shuf);
   Value *NewExt = Builder.CreateExtractElement(VecLogic, CheapIndex);
   replaceValue(I, *NewExt);
   ++NumVecCmpBO;
   return true;
 }
 
+// Check if memory loc modified between two instrs in the same BB
+static bool isMemModifiedBetween(BasicBlock::iterator Begin,
+                                 BasicBlock::iterator End,
+                                 const MemoryLocation &Loc, AAResults &AA) {
+  for (BasicBlock::iterator BBI = Begin; BBI != End; ++BBI)
+    if (isModSet(AA.getModRefInfo(&*BBI, Loc)))
+      return true;
+  return false;
+}
+
+// Combine patterns like:
+//   %0 = load <4 x i32>, <4 x i32>* %a
+//   %1 = insertelement <4 x i32> %0, i32 %b, i32 1
+//   store <4 x i32> %1, <4 x i32>* %a
+// to:
+//   %0 = bitcast <4 x i32>* %a to i32*
+//   %1 = getelementptr inbounds i32, i32* %0, i64 0, i64 1
+//   store i32 %b, i32* %1
+bool VectorCombine::foldSingleElementStore(Instruction &I) {
+  StoreInst *SI = dyn_cast<StoreInst>(&I);
+  if (SI == nullptr || !SI->isSimple() ||
+      !SI->getValueOperand()->getType()->isVectorTy())
+    return false;
+
+  // TODO: Combine more complicated patterns (multiple insert) by referencing
+  // TargetTransformInfo.
+  Instruction *Source;
+  Value *NewElement;
+  Constant *Idx;
+  if (!match(SI->getValueOperand(),
+             m_InsertElt(m_Instruction(Source), m_Value(NewElement),
+                         m_Constant(Idx))))
+    return false;
+
+  if (auto *Load = dyn_cast<LoadInst>(Source)) {
+    Value *SrcAddr = Load->getPointerOperand()->stripPointerCasts();
+    // Don't optimize for atomic/volatile load or stores.
+    if (!Load->isSimple() || Load->getParent() != SI->getParent() ||
+        SrcAddr != SI->getPointerOperand()->stripPointerCasts() ||
+        isMemModifiedBetween(Load->getIterator(), SI->getIterator(),
+                             MemoryLocation::get(SI), AA))
+      return false;
+
+    Type *ElePtrType = NewElement->getType()->getPointerTo();
+    Value *ElePtr =
+        Builder.CreatePointerCast(SI->getPointerOperand(), ElePtrType);
+    Value *GEP = Builder.CreateInBoundsGEP(NewElement->getType(), ElePtr, Idx);
+    StoreInst *NSI = Builder.CreateStore(NewElement, GEP);
+    NSI->copyMetadata(*SI, {LLVMContext::MD_nontemporal});
+    replaceValue(I, *NSI);
+    I.eraseFromParent();
+    return true;
+  }
+
+  return false;
+}
+
 /// This is the entry point for all transforms. Pass manager differences are
 /// handled in the callers of this function.
 bool VectorCombine::run() {
   if (DisableVectorCombine)
     return false;
 
   // Don't attempt vectorization if the target does not support vectors.
   if (!TTI.getNumberOfRegisters(TTI.getRegisterClassForType(/*Vector*/ true)))
     return false;
 
   bool MadeChange = false;
   for (BasicBlock &BB : F) {
     // Ignore unreachable basic blocks.
     if (!DT.isReachableFromEntry(&BB))
       continue;
     // Do not delete instructions under here and invalidate the iterator.
     // Walk the block forwards to enable simple iterative chains of transforms.
     // TODO: It could be more efficient to remove dead instructions
     //       iteratively in this loop rather than waiting until the end.
-    for (Instruction &I : BB) {
+    for (Instruction &I : make_early_inc_range(BB)) {
       if (isa<DbgInfoIntrinsic>(I))
         continue;
       Builder.SetInsertPoint(&I);
       MadeChange |= vectorizeLoadInsert(I);
       MadeChange |= foldExtractExtract(I);
       MadeChange |= foldBitcastShuf(I);
       MadeChange |= scalarizeBinopOrCmp(I);
       MadeChange |= foldExtractedCmps(I);
+      MadeChange |= foldSingleElementStore(I);
     }
   }
 
   // We're done with transforms, so remove dead instructions.
   if (MadeChange)
     for (BasicBlock &BB : F)
       SimplifyInstructionsInBlock(&BB);
 
public:
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequired<TargetTransformInfoWrapperPass>();
     AU.setPreservesCFG();
     AU.addPreserved<DominatorTreeWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
     AU.addPreserved<AAResultsWrapperPass>();
     AU.addPreserved<BasicAAWrapperPass>();
     FunctionPass::getAnalysisUsage(AU);
   }
 
   bool runOnFunction(Function &F) override {
     if (skipFunction(F))
       return false;
     auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
     auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-    VectorCombine Combiner(F, TTI, DT);
+    auto &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
+    VectorCombine Combiner(F, TTI, DT, AA);
     return Combiner.run();
   }
 };
 } // namespace
 
 char VectorCombineLegacyPass::ID = 0;
 INITIALIZE_PASS_BEGIN(VectorCombineLegacyPass, "vector-combine",
                       "Optimize scalar/vector ops", false,
                       false)
 INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
 INITIALIZE_PASS_END(VectorCombineLegacyPass, "vector-combine",
                     "Optimize scalar/vector ops", false, false)
 Pass *llvm::createVectorCombinePass() {
   return new VectorCombineLegacyPass();
 }
 
 PreservedAnalyses VectorCombinePass::run(Function &F,
                                          FunctionAnalysisManager &FAM) {
   TargetTransformInfo &TTI = FAM.getResult<TargetIRAnalysis>(F);
   DominatorTree &DT = FAM.getResult<DominatorTreeAnalysis>(F);
-  VectorCombine Combiner(F, TTI, DT);
+  AAResults &AA = FAM.getResult<AAManager>(F);
+  VectorCombine Combiner(F, TTI, DT, AA);
   if (!Combiner.run())
     return PreservedAnalyses::all();
   PreservedAnalyses PA;
   PA.preserveSet<CFGAnalyses>();
   PA.preserve<GlobalsAA>();
   PA.preserve<AAManager>();
   PA.preserve<BasicAA>();
   return PA;
 }

llvm/test/Transforms/VectorCombine/X86/load-insert-store.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -S -instcombine < %s | FileCheck %s
+; RUN: opt -S -vector-combine -data-layout=e < %s | FileCheck %s
+; RUN: opt -S -vector-combine -data-layout=E < %s | FileCheck %s
 
 define void @insert_store(<16 x i8>* %q, i8 zeroext %s) {
 ; CHECK-LABEL: @insert_store(
 ; 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:%.*]] = bitcast <16 x i8>* [[Q:%.*]] to i8*
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i8, i8* [[TMP0]], i32 3
+; CHECK-NEXT:    store i8 [[S:%.*]], i8* [[TMP1]], align 1
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %0 = load <16 x i8>, <16 x i8>* %q
   %vecins = insertelement <16 x i8> %0, i8 %s, i32 3
   store <16 x i8> %vecins, <16 x i8>* %q
   ret void
 }
 
-define void @single_shuffle_store(<4 x i32>* %a, i32 %b) {
-; CHECK-LABEL: @single_shuffle_store(
+define void @insert_store_i16(<8 x i16>* %q, i16 zeroext %s) {
+; CHECK-LABEL: @insert_store_i16(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = load <4 x i32>, <4 x i32>* [[A:%.*]], align 16
-; CHECK-NEXT:    [[TMP1:%.*]] = insertelement <4 x i32> [[TMP0]], i32 [[B:%.*]], i32 1
-; CHECK-NEXT:    store <4 x i32> [[TMP1]], <4 x i32>* [[A]], align 16, !nontemporal !0
+; CHECK-NEXT:    [[TMP0:%.*]] = bitcast <8 x i16>* [[Q:%.*]] to i16*
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i16, i16* [[TMP0]], i32 3
+; CHECK-NEXT:    store i16 [[S:%.*]], i16* [[TMP1]], align 2
 ; CHECK-NEXT:    ret void
 ;
 entry:
-  %0 = load <4 x i32>, <4 x i32>* %a
-  %1 = insertelement <4 x i32> %0, i32 %b, i32 1
-  %2 = shufflevector <4 x i32> %0, <4 x i32> %1, <4 x i32> <i32 0, i32 5, i32 2, i32 3>
-  store <4 x i32> %2, <4 x i32>* %a, !nontemporal !0
+  %0 = load <8 x i16>, <8 x i16>* %q
+  %vecins = insertelement <8 x i16> %0, i16 %s, i32 3
+  store <8 x i16> %vecins, <8 x i16>* %q
   ret void
 }
 
 define void @volatile_update(<16 x i8>* %q, <16 x i8>* %p, i8 zeroext %s) {
 ; CHECK-LABEL: @volatile_update(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = load <16 x i8>, <16 x i8>* [[Q:%.*]], align 16
 ; CHECK-NEXT:    [[VECINS0:%.*]] = insertelement <16 x i8> [[TMP0]], i8 [[S:%.*]], i32 3

 ;
 entry:
   %ld = load <16 x i8>, <16 x i8>* %p
   %ins = insertelement <16 x i8> %ld, i8 %s, i32 3
   store <16 x i8> %ins, <16 x i8>* %q
   ret void
 }
 
+; We can't transform if any instr could modify memory in between.
+; Here p and q may alias, so we can't remove the load.
+; r is impossible to alias with others, so it's safe to transform.
 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(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[LD:%.*]] = load <16 x i8>, <16 x i8>* [[P:%.*]], align 16
 ; CHECK-NEXT:    store <16 x i8> zeroinitializer, <16 x i8>* [[Q:%.*]], align 16
 ; CHECK-NEXT:    [[INS:%.*]] = insertelement <16 x i8> [[LD]], i8 [[S:%.*]], i32 3
 ; CHECK-NEXT:    store <16 x i8> [[INS]], <16 x i8>* [[P]], align 16
-; CHECK-NEXT:    [[LD2:%.*]] = load <16 x i8>, <16 x i8>* [[Q]], align 16
 ; CHECK-NEXT:    store <16 x i8> zeroinitializer, <16 x i8>* [[R:%.*]], align 16
-; CHECK-NEXT:    [[INS2:%.*]] = insertelement <16 x i8> [[LD2]], i8 [[S]], i32 7
-; CHECK-NEXT:    store <16 x i8> [[INS2]], <16 x i8>* [[Q]], align 16
+; CHECK-NEXT:    [[TMP0:%.*]] = bitcast <16 x i8>* [[Q]] to i8*
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i8, i8* [[TMP0]], i32 7
+; CHECK-NEXT:    store i8 [[S]], i8* [[TMP1]], align 1
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %ld = load <16 x i8>, <16 x i8>* %p
   store <16 x i8> zeroinitializer, <16 x i8>* %q
   %ins = insertelement <16 x i8> %ld, i8 %s, i32 3
   store <16 x i8> %ins, <16 x i8>* %p
 
   %ld2 = load <16 x i8>, <16 x i8>* %q
   store <16 x i8> zeroinitializer, <16 x i8>* %r
   %ins2 = insertelement <16 x i8> %ld2, i8 %s, i32 7
   store <16 x i8> %ins2, <16 x i8>* %q
   ret void
 }
 
+; Check cases when calls may modify memory
+define void @insert_store_with_call(<16 x i8>* %p, <16 x i8>* %q, i8 %s) {
+; CHECK-LABEL: @insert_store_with_call(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[LD:%.*]] = load <16 x i8>, <16 x i8>* [[P:%.*]], align 16
+; CHECK-NEXT:    call void @maywrite(<16 x i8>* [[P]])
+; CHECK-NEXT:    [[INS:%.*]] = insertelement <16 x i8> [[LD]], i8 [[S:%.*]], i32 3
+; CHECK-NEXT:    store <16 x i8> [[INS]], <16 x i8>* [[P]], align 16
+; CHECK-NEXT:    call void @foo()
+; CHECK-NEXT:    call void @nowrite(<16 x i8>* [[P]])
+; CHECK-NEXT:    [[TMP0:%.*]] = bitcast <16 x i8>* [[P]] to i8*
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i8, i8* [[TMP0]], i32 7
+; CHECK-NEXT:    store i8 [[S]], i8* [[TMP1]], align 1
+; CHECK-NEXT:    ret void
+;
+entry:
+  %ld = load <16 x i8>, <16 x i8>* %p
+  call void @maywrite(<16 x i8>* %p)
+  %ins = insertelement <16 x i8> %ld, i8 %s, i32 3
+  store <16 x i8> %ins, <16 x i8>* %p
+  call void @foo()  ; Barrier
+  %ld2 = load <16 x i8>, <16 x i8>* %p
+  call void @nowrite(<16 x i8>* %p)
+  %ins2 = insertelement <16 x i8> %ld2, i8 %s, i32 7
+  store <16 x i8> %ins2, <16 x i8>* %p
+  ret void
+}
+
+declare void @foo()
+declare void @maywrite(<16 x i8>*)
+declare void @nowrite(<16 x i8>*) readonly
+
 !0 = !{}