[LSV] Use Instruction*s rather than Value*s where possible.

[jlebar]

Summary

Given the crash in D22878, this patch converts the load/store vectorizer
to use explicit Instruction*s wherever possible. This is an overall
simplification and should be an improvement in safety, as we have fewer
naked cast<>s, and now where we use Value*, we really mean something
different from Instruction*.

This patch also gets rid of some cast<>s around Value*s returned by
Builder. Given that Builder constant-folds everything, we can't assume
much about what we get out of it.

One downside of this patch is that we have to copy our chain before
calling propagateMetadata. But I don't think this is a big deal, as our
chains are very small (usually 2 or 4 elems).

[asbirlea]

I had mixed feeling about some of the changes, but considering the overall removal of casts, this looks worth doing.

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

846 ↗

(On Diff #65809)

When is this not a store instruction? Does it affect in any way the decision to vectorize (it's kinda late in the flow..)?

973 ↗

(On Diff #65809)

Same as for stores. When is this not a load and what does that mean? (consequences when used below? I honestly have no idea...)
Perhaps add a comment explaining this?

[jlebar]

Remove spurrious checks.

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

846 ↗

(On Diff #65809)

Looking at the Builder code I don't think it can do that today, for a load or a store.

Normally I'd say I don't want to rely on the existing behavior -- having a load of a constant return a constant doesn't seem unreasonable -- but I'm sure that any attempt to change the behavior of Builder would break tons of other people too. So, we might as well depend on it too.

Changed.

[asbirlea]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

971 ↗

(On Diff #65828)

Nit: move comment to the cast?

llvm/trunk/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

 STATISTIC(NumVectorInstructions, "Number of vector accesses generated");
 STATISTIC(NumScalarsVectorized, "Number of scalar accesses vectorized");
 
 namespace {
 
 // TODO: Remove this
 static const unsigned TargetBaseAlign = 4;
 
-typedef SmallVector<Value *, 8> ValueList;
-typedef MapVector<Value *, ValueList> ValueListMap;
+typedef SmallVector<Instruction *, 8> InstrList;
+typedef MapVector<Value *, InstrList> InstrListMap;
 
 class Vectorizer {
   Function &F;
   AliasAnalysis &AA;
   DominatorTree &DT;
   ScalarEvolution &SE;
   TargetTransformInfo &TTI;
   const DataLayout &DL;
private:
   bool isConsecutiveAccess(Value *A, Value *B);
 
   /// After vectorization, reorder the instructions that I depends on
   /// (the instructions defining its operands), to ensure they dominate I.
   void reorder(Instruction *I);
 
   /// Returns the first and the last instructions in Chain.
   std::pair<BasicBlock::iterator, BasicBlock::iterator>
-  getBoundaryInstrs(ArrayRef<Value *> Chain);
+  getBoundaryInstrs(ArrayRef<Instruction *> Chain);
 
   /// Erases the original instructions after vectorizing.
-  void eraseInstructions(ArrayRef<Value *> Chain);
+  void eraseInstructions(ArrayRef<Instruction *> Chain);
 
   /// "Legalize" the vector type that would be produced by combining \p
   /// ElementSizeBits elements in \p Chain. Break into two pieces such that the
   /// total size of each piece is 1, 2 or a multiple of 4 bytes. \p Chain is
   /// expected to have more than 4 elements.
-  std::pair<ArrayRef<Value *>, ArrayRef<Value *>>
-  splitOddVectorElts(ArrayRef<Value *> Chain, unsigned ElementSizeBits);
+  std::pair<ArrayRef<Instruction *>, ArrayRef<Instruction *>>
+  splitOddVectorElts(ArrayRef<Instruction *> Chain, unsigned ElementSizeBits);
 
   /// Finds the largest prefix of Chain that's vectorizable, checking for
   /// intervening instructions which may affect the memory accessed by the
   /// instructions within Chain.
   ///
   /// The elements of \p Chain must be all loads or all stores and must be in
   /// address order.
-  ArrayRef<Value *> getVectorizablePrefix(ArrayRef<Value *> Chain);
+  ArrayRef<Instruction *> getVectorizablePrefix(ArrayRef<Instruction *> Chain);
 
   /// Collects load and store instructions to vectorize.
-  std::pair<ValueListMap, ValueListMap> collectInstructions(BasicBlock *BB);
+  std::pair<InstrListMap, InstrListMap> collectInstructions(BasicBlock *BB);
 
-  /// Processes the collected instructions, the \p Map. The elements of \p Map
+  /// Processes the collected instructions, the \p Map. The values of \p Map
   /// should be all loads or all stores.
-  bool vectorizeChains(ValueListMap &Map);
+  bool vectorizeChains(InstrListMap &Map);
 
   /// Finds the load/stores to consecutive memory addresses and vectorizes them.
-  bool vectorizeInstructions(ArrayRef<Value *> Instrs);
+  bool vectorizeInstructions(ArrayRef<Instruction *> Instrs);
 
   /// Vectorizes the load instructions in Chain.
-  bool vectorizeLoadChain(ArrayRef<Value *> Chain,
-                          SmallPtrSet<Value *, 16> *InstructionsProcessed);
+  bool
+  vectorizeLoadChain(ArrayRef<Instruction *> Chain,
+                     SmallPtrSet<Instruction *, 16> *InstructionsProcessed);
 
   /// Vectorizes the store instructions in Chain.
-  bool vectorizeStoreChain(ArrayRef<Value *> Chain,
-                           SmallPtrSet<Value *, 16> *InstructionsProcessed);
+  bool
+  vectorizeStoreChain(ArrayRef<Instruction *> Chain,
+                      SmallPtrSet<Instruction *, 16> *InstructionsProcessed);
 
   /// Check if this load/store access is misaligned accesses
   bool accessIsMisaligned(unsigned SzInBytes, unsigned AddressSpace,
                           unsigned Alignment);
 };
 
 class LoadStoreVectorizer : public FunctionPass {
 public:
INITIALIZE_PASS_END(LoadStoreVectorizer, DEBUG_TYPE,
                     "Vectorize load and store instructions", false, false)
 
 char LoadStoreVectorizer::ID = 0;
 
 Pass *llvm::createLoadStoreVectorizerPass() {
   return new LoadStoreVectorizer();
 }
 
+// The real propagateMetadata expects a SmallVector<Value*>, but we deal in
+// vectors of Instructions.
+static void propagateMetadata(Instruction *I, ArrayRef<Instruction *> IL) {
+  SmallVector<Value *, 8> VL(IL.begin(), IL.end());
+  propagateMetadata(I, VL);
+}
+
 bool LoadStoreVectorizer::runOnFunction(Function &F) {
   // Don't vectorize when the attribute NoImplicitFloat is used.
   if (skipFunction(F) || F.hasFnAttribute(Attribute::NoImplicitFloat))
     return false;
 
   AliasAnalysis &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
   DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
   ScalarEvolution &SE = getAnalysis<ScalarEvolutionWrapperPass>().getSE();
   TargetTransformInfo &TTI =
       getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
 
   Vectorizer V(F, AA, DT, SE, TTI);
   return V.run();
 }
 
 // Vectorizer Implementation
 bool Vectorizer::run() {
   bool Changed = false;
 
   // Scan the blocks in the function in post order.
   for (BasicBlock *BB : post_order(&F)) {
-    ValueListMap LoadRefs, StoreRefs;
+    InstrListMap LoadRefs, StoreRefs;
     std::tie(LoadRefs, StoreRefs) = collectInstructions(BB);
     Changed |= vectorizeChains(LoadRefs);
     Changed |= vectorizeChains(StoreRefs);
   }
 
   return Changed;
 }
 
for (auto BBI = I->getIterator(), E = I->getParent()->end(); BBI != E;
     Instruction *IM = &*BBI;
     --BBI;
     IM->removeFromParent();
     IM->insertBefore(I);
   }
 }
 
 std::pair<BasicBlock::iterator, BasicBlock::iterator>
-Vectorizer::getBoundaryInstrs(ArrayRef<Value *> Chain) {
-  Instruction *C0 = cast<Instruction>(Chain[0]);
+Vectorizer::getBoundaryInstrs(ArrayRef<Instruction *> Chain) {
+  Instruction *C0 = Chain[0];
   BasicBlock::iterator FirstInstr = C0->getIterator();
   BasicBlock::iterator LastInstr = C0->getIterator();
 
   BasicBlock *BB = C0->getParent();
   unsigned NumFound = 0;
   for (Instruction &I : *BB) {
     if (!is_contained(Chain, &I))
       continue;
 
     ++NumFound;
     if (NumFound == 1) {
       FirstInstr = I.getIterator();
     }
     if (NumFound == Chain.size()) {
       LastInstr = I.getIterator();
       break;
     }
   }
 
   // Range is [first, last).
   return std::make_pair(FirstInstr, ++LastInstr);
 }
 
-void Vectorizer::eraseInstructions(ArrayRef<Value *> Chain) {
+void Vectorizer::eraseInstructions(ArrayRef<Instruction *> Chain) {
   SmallVector<Instruction *, 16> Instrs;
-  for (Value *V : Chain) {
-    Value *PtrOperand = getPointerOperand(V);
+  for (Instruction *I : Chain) {
+    Value *PtrOperand = getPointerOperand(I);
     assert(PtrOperand && "Instruction must have a pointer operand.");
-    Instrs.push_back(cast<Instruction>(V));
+    Instrs.push_back(I);
     if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(PtrOperand))
       Instrs.push_back(GEP);
   }
 
   // Erase instructions.
-  for (Value *V : Instrs) {
-    Instruction *Instr = cast<Instruction>(V);
-    if (Instr->use_empty())
-      Instr->eraseFromParent();
-  }
+  for (Instruction *I : Instrs)
+    if (I->use_empty())
+      I->eraseFromParent();
 }
 
-std::pair<ArrayRef<Value *>, ArrayRef<Value *>>
-Vectorizer::splitOddVectorElts(ArrayRef<Value *> Chain,
+std::pair<ArrayRef<Instruction *>, ArrayRef<Instruction *>>
+Vectorizer::splitOddVectorElts(ArrayRef<Instruction *> Chain,
                                unsigned ElementSizeBits) {
   unsigned ElemSizeInBytes = ElementSizeBits / 8;
   unsigned SizeInBytes = ElemSizeInBytes * Chain.size();
   unsigned NumRight = (SizeInBytes % 4) / ElemSizeInBytes;
   unsigned NumLeft = Chain.size() - NumRight;
   return std::make_pair(Chain.slice(0, NumLeft), Chain.slice(NumLeft));
 }
 
-ArrayRef<Value *> Vectorizer::getVectorizablePrefix(ArrayRef<Value *> Chain) {
+ArrayRef<Instruction *>
+Vectorizer::getVectorizablePrefix(ArrayRef<Instruction *> Chain) {
   // These are in BB order, unlike Chain, which is in address order.
-  SmallVector<std::pair<Value *, unsigned>, 16> MemoryInstrs;
-  SmallVector<std::pair<Value *, unsigned>, 16> ChainInstrs;
+  SmallVector<std::pair<Instruction *, unsigned>, 16> MemoryInstrs;
+  SmallVector<std::pair<Instruction *, unsigned>, 16> ChainInstrs;
 
   bool IsLoadChain = isa<LoadInst>(Chain[0]);
   DEBUG({
-    for (Value *V : Chain) {
+    for (Instruction *I : Chain) {
       if (IsLoadChain)
-        assert(isa<LoadInst>(V) &&
+        assert(isa<LoadInst>(I) &&
                "All elements of Chain must be loads, or all must be stores.");
       else
-        assert(isa<StoreInst>(V) &&
+        assert(isa<StoreInst>(I) &&
                "All elements of Chain must be loads, or all must be stores.");
     }
   });
 
   unsigned InstrIdx = 0;
   for (Instruction &I : make_range(getBoundaryInstrs(Chain))) {
     ++InstrIdx;
     if (isa<LoadInst>(I) || isa<StoreInst>(I)) {
if (isa<LoadInst>(I) || isa<StoreInst>(I)) {
       break;
     }
   }
 
   // Loop until we find an instruction in ChainInstrs that we can't vectorize.
   unsigned ChainInstrIdx, ChainInstrsLen;
   for (ChainInstrIdx = 0, ChainInstrsLen = ChainInstrs.size();
        ChainInstrIdx < ChainInstrsLen; ++ChainInstrIdx) {
-    Value *ChainInstr = ChainInstrs[ChainInstrIdx].first;
+    Instruction *ChainInstr = ChainInstrs[ChainInstrIdx].first;
     unsigned ChainInstrLoc = ChainInstrs[ChainInstrIdx].second;
     bool AliasFound = false;
     for (auto EntryMem : MemoryInstrs) {
-      Value *MemInstr = EntryMem.first;
+      Instruction *MemInstr = EntryMem.first;
       unsigned MemInstrLoc = EntryMem.second;
       if (isa<LoadInst>(MemInstr) && isa<LoadInst>(ChainInstr))
         continue;
 
       // We can ignore the alias as long as the load comes before the store,
       // because that means we won't be moving the load past the store to
       // vectorize it (the vectorized load is inserted at the location of the
       // first load in the chain).
       if (isa<StoreInst>(MemInstr) && isa<LoadInst>(ChainInstr) &&
           ChainInstrLoc < MemInstrLoc)
         continue;
 
       // Same case, but in reverse.
       if (isa<LoadInst>(MemInstr) && isa<StoreInst>(ChainInstr) &&
           ChainInstrLoc > MemInstrLoc)
         continue;
 
-      Instruction *M0 = cast<Instruction>(MemInstr);
-      Instruction *M1 = cast<Instruction>(ChainInstr);
-
-      if (!AA.isNoAlias(MemoryLocation::get(M0), MemoryLocation::get(M1))) {
+      if (!AA.isNoAlias(MemoryLocation::get(MemInstr),
+                        MemoryLocation::get(ChainInstr))) {
         DEBUG({
-          Value *Ptr0 = getPointerOperand(M0);
-          Value *Ptr1 = getPointerOperand(M1);
           dbgs() << "LSV: Found alias:\n"
                     "  Aliasing instruction and pointer:\n"
                  << "  " << *MemInstr << '\n'
-                 << "  " << *Ptr0 << '\n'
+                 << "  " << *getPointerOperand(MemInstr) << '\n'
                  << "  Aliased instruction and pointer:\n"
                  << "  " << *ChainInstr << '\n'
-                 << "  " << *Ptr1 << '\n';
+                 << "  " << *getPointerOperand(ChainInstr) << '\n';
         });
         AliasFound = true;
         break;
       }
     }
     if (AliasFound)
       break;
   }
 
   // Find the largest prefix of Chain whose elements are all in
   // ChainInstrs[0, ChainInstrIdx).  This is the largest vectorizable prefix of
   // Chain.  (Recall that Chain is in address order, but ChainInstrs is in BB
   // order.)
   auto VectorizableChainInstrs =
       makeArrayRef(ChainInstrs.data(), ChainInstrIdx);
   unsigned ChainIdx, ChainLen;
   for (ChainIdx = 0, ChainLen = Chain.size(); ChainIdx < ChainLen; ++ChainIdx) {
-    Value *V = Chain[ChainIdx];
+    Instruction *I = Chain[ChainIdx];
     if (!any_of(VectorizableChainInstrs,
-                [V](std::pair<Value *, unsigned> CI) { return V == CI.first; }))
+                [I](std::pair<Instruction *, unsigned> CI) {
+                  return I == CI.first;
+                }))
       break;
   }
   return Chain.slice(0, ChainIdx);
 }
 
-std::pair<ValueListMap, ValueListMap>
+std::pair<InstrListMap, InstrListMap>
 Vectorizer::collectInstructions(BasicBlock *BB) {
-  ValueListMap LoadRefs;
-  ValueListMap StoreRefs;
+  InstrListMap LoadRefs;
+  InstrListMap StoreRefs;
 
   for (Instruction &I : *BB) {
     if (!I.mayReadOrWriteMemory())
       continue;
 
     if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
       if (!LI->isSimple())
         continue;
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
       unsigned VecRegSize = TTI.getLoadStoreVecRegBitWidth(AS);
 
       // No point in looking at these if they're too big to vectorize.
       if (TySize > VecRegSize / 2)
         continue;
 
       // Make sure all the users of a vector are constant-index extracts.
       if (isa<VectorType>(Ty) && !all_of(LI->users(), [LI](const User *U) {
-            const Instruction *UI = cast<Instruction>(U);
-            return isa<ExtractElementInst>(UI) &&
-                   isa<ConstantInt>(UI->getOperand(1));
+            const ExtractElementInst *EEI = dyn_cast<ExtractElementInst>(U);
+            return EEI && isa<ConstantInt>(EEI->getOperand(1));
           }))
         continue;
 
       // TODO: Target hook to filter types.
 
       // Save the load locations.
       Value *ObjPtr = GetUnderlyingObject(Ptr, DL);
       LoadRefs[ObjPtr].push_back(LI);
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
 
       Value *Ptr = SI->getPointerOperand();
       unsigned AS = Ptr->getType()->getPointerAddressSpace();
       unsigned VecRegSize = TTI.getLoadStoreVecRegBitWidth(AS);
       if (TySize > VecRegSize / 2)
         continue;
 
       if (isa<VectorType>(Ty) && !all_of(SI->users(), [SI](const User *U) {
-            const Instruction *UI = cast<Instruction>(U);
-            return isa<ExtractElementInst>(UI) &&
-                   isa<ConstantInt>(UI->getOperand(1));
+            const ExtractElementInst *EEI = dyn_cast<ExtractElementInst>(U);
+            return EEI && isa<ConstantInt>(EEI->getOperand(1));
           }))
         continue;
 
       // Save store location.
       Value *ObjPtr = GetUnderlyingObject(Ptr, DL);
       StoreRefs[ObjPtr].push_back(SI);
     }
   }
 
   return {LoadRefs, StoreRefs};
 }
 
-bool Vectorizer::vectorizeChains(ValueListMap &Map) {
+bool Vectorizer::vectorizeChains(InstrListMap &Map) {
   bool Changed = false;
 
-  for (const std::pair<Value *, ValueList> &Chain : Map) {
+  for (const std::pair<Value *, InstrList> &Chain : Map) {
     unsigned Size = Chain.second.size();
     if (Size < 2)
       continue;
 
     DEBUG(dbgs() << "LSV: Analyzing a chain of length " << Size << ".\n");
 
     // Process the stores in chunks of 64.
     for (unsigned CI = 0, CE = Size; CI < CE; CI += 64) {
       unsigned Len = std::min<unsigned>(CE - CI, 64);
-      ArrayRef<Value *> Chunk(&Chain.second[CI], Len);
+      ArrayRef<Instruction *> Chunk(&Chain.second[CI], Len);
       Changed |= vectorizeInstructions(Chunk);
     }
   }
 
   return Changed;
 }
 
-bool Vectorizer::vectorizeInstructions(ArrayRef<Value *> Instrs) {
+bool Vectorizer::vectorizeInstructions(ArrayRef<Instruction *> Instrs) {
   DEBUG(dbgs() << "LSV: Vectorizing " << Instrs.size() << " instructions.\n");
   SmallSetVector<int, 16> Heads, Tails;
   int ConsecutiveChain[64];
 
   // Do a quadratic search on all of the given stores and find all of the pairs
   // of stores that follow each other.
   for (int i = 0, e = Instrs.size(); i < e; ++i) {
     ConsecutiveChain[i] = -1;
for (int j = e - 1; j >= 0; --j) {
         Tails.insert(j);
         Heads.insert(i);
         ConsecutiveChain[i] = j;
       }
     }
   }
 
   bool Changed = false;
-  SmallPtrSet<Value *, 16> InstructionsProcessed;
+  SmallPtrSet<Instruction *, 16> InstructionsProcessed;
 
   for (int Head : Heads) {
     if (InstructionsProcessed.count(Instrs[Head]))
       continue;
     bool longerChainExists = false;
     for (unsigned TIt = 0; TIt < Tails.size(); TIt++)
       if (Head == Tails[TIt] &&
           !InstructionsProcessed.count(Instrs[Heads[TIt]])) {
         longerChainExists = true;
         break;
       }
     if (longerChainExists)
       continue;
 
     // We found an instr that starts a chain. Now follow the chain and try to
     // vectorize it.
-    SmallVector<Value *, 16> Operands;
+    SmallVector<Instruction *, 16> Operands;
     int I = Head;
     while (I != -1 && (Tails.count(I) || Heads.count(I))) {
       if (InstructionsProcessed.count(Instrs[I]))
         break;
 
       Operands.push_back(Instrs[I]);
       I = ConsecutiveChain[I];
     }
 
     bool Vectorized = false;
     if (isa<LoadInst>(*Operands.begin()))
       Vectorized = vectorizeLoadChain(Operands, &InstructionsProcessed);
     else
       Vectorized = vectorizeStoreChain(Operands, &InstructionsProcessed);
 
     Changed |= Vectorized;
   }
 
   return Changed;
 }
 
 bool Vectorizer::vectorizeStoreChain(
-    ArrayRef<Value *> Chain, SmallPtrSet<Value *, 16> *InstructionsProcessed) {
+    ArrayRef<Instruction *> Chain,
+    SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
   StoreInst *S0 = cast<StoreInst>(Chain[0]);
 
   // If the vector has an int element, default to int for the whole load.
   Type *StoreTy;
-  for (const auto &V : Chain) {
-    StoreTy = cast<StoreInst>(V)->getValueOperand()->getType();
+  for (Instruction *I : Chain) {
+    StoreTy = cast<StoreInst>(I)->getValueOperand()->getType();
     if (StoreTy->isIntOrIntVectorTy())
       break;
 
     if (StoreTy->isPtrOrPtrVectorTy()) {
       StoreTy = Type::getIntNTy(F.getParent()->getContext(),
                                 DL.getTypeSizeInBits(StoreTy));
       break;
     }
   }
 
   unsigned Sz = DL.getTypeSizeInBits(StoreTy);
   unsigned AS = S0->getPointerAddressSpace();
   unsigned VecRegSize = TTI.getLoadStoreVecRegBitWidth(AS);
   unsigned VF = VecRegSize / Sz;
   unsigned ChainSize = Chain.size();
 
   if (!isPowerOf2_32(Sz) || VF < 2 || ChainSize < 2) {
     InstructionsProcessed->insert(Chain.begin(), Chain.end());
     return false;
   }
 
-  ArrayRef<Value *> NewChain = getVectorizablePrefix(Chain);
+  ArrayRef<Instruction *> NewChain = getVectorizablePrefix(Chain);
   if (NewChain.empty()) {
     // No vectorization possible.
     InstructionsProcessed->insert(Chain.begin(), Chain.end());
     return false;
   }
   if (NewChain.size() == 1) {
     // Failed after the first instruction. Discard it and try the smaller chain.
     InstructionsProcessed->insert(NewChain.front());
if (ChainSize > VF) {
     DEBUG(dbgs() << "LSV: Vector factor is too big."
                     " Creating two separate arrays.\n");
     return vectorizeStoreChain(Chain.slice(0, VF), InstructionsProcessed) |
            vectorizeStoreChain(Chain.slice(VF), InstructionsProcessed);
   }
 
   DEBUG({
     dbgs() << "LSV: Stores to vectorize:\n";
-    for (Value *V : Chain)
-      dbgs() << "  " << *V << "\n";
+    for (Instruction *I : Chain)
+      dbgs() << "  " << *I << "\n";
   });
 
   // We won't try again to vectorize the elements of the chain, regardless of
   // whether we succeed below.
   InstructionsProcessed->insert(Chain.begin(), Chain.end());
 
   // Check alignment restrictions.
   unsigned Alignment = getAlignment(S0);
for (unsigned I = 0, E = Chain.size(); I != E; ++I) {
             Builder.CreateBitOrPointerCast(Extract, StoreTy->getScalarType());
 
       Value *Insert =
           Builder.CreateInsertElement(Vec, Extract, Builder.getInt32(I));
       Vec = Insert;
     }
   }
 
-  Value *Bitcast =
-      Builder.CreateBitCast(S0->getPointerOperand(), VecTy->getPointerTo(AS));
-  StoreInst *SI = cast<StoreInst>(Builder.CreateStore(Vec, Bitcast));
+  // This cast is safe because Builder.CreateStore() always creates a bona fide
+  // StoreInst.
+  StoreInst *SI = cast<StoreInst>(
+      Builder.CreateStore(Vec, Builder.CreateBitCast(S0->getPointerOperand(),
+                                                     VecTy->getPointerTo(AS))));
   propagateMetadata(SI, Chain);
   SI->setAlignment(Alignment);
 
   eraseInstructions(Chain);
   ++NumVectorInstructions;
   NumScalarsVectorized += Chain.size();
   return true;
 }
 
 bool Vectorizer::vectorizeLoadChain(
-    ArrayRef<Value *> Chain, SmallPtrSet<Value *, 16> *InstructionsProcessed) {
+    ArrayRef<Instruction *> Chain,
+    SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
   LoadInst *L0 = cast<LoadInst>(Chain[0]);
 
   // If the vector has an int element, default to int for the whole load.
   Type *LoadTy;
   for (const auto &V : Chain) {
     LoadTy = cast<LoadInst>(V)->getType();
     if (LoadTy->isIntOrIntVectorTy())
       break;
bool Vectorizer::vectorizeLoadChain(
   unsigned VF = VecRegSize / Sz;
   unsigned ChainSize = Chain.size();
 
   if (!isPowerOf2_32(Sz) || VF < 2 || ChainSize < 2) {
     InstructionsProcessed->insert(Chain.begin(), Chain.end());
     return false;
   }
 
-  ArrayRef<Value *> NewChain = getVectorizablePrefix(Chain);
+  ArrayRef<Instruction *> NewChain = getVectorizablePrefix(Chain);
   if (NewChain.empty()) {
     // No vectorization possible.
     InstructionsProcessed->insert(Chain.begin(), Chain.end());
     return false;
   }
   if (NewChain.size() == 1) {
     // Failed after the first instruction. Discard it and try the smaller chain.
     InstructionsProcessed->insert(NewChain.front());
if (AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V)) {
       Alignment = TargetBaseAlign;
     } else {
       return false;
     }
   }
 
   DEBUG({
     dbgs() << "LSV: Loads to vectorize:\n";
-    for (Value *V : Chain)
-      V->dump();
+    for (Instruction *I : Chain)
+      I->dump();
   });
 
   // getVectorizablePrefix already computed getBoundaryInstrs.  The value of
   // Last may have changed since then, but the value of First won't have.  If it
   // matters, we could compute getBoundaryInstrs only once and reuse it here.
   BasicBlock::iterator First, Last;
   std::tie(First, Last) = getBoundaryInstrs(Chain);
   Builder.SetInsertPoint(&*First);
 
   Value *Bitcast =
       Builder.CreateBitCast(L0->getPointerOperand(), VecTy->getPointerTo(AS));
-
+  // This cast is safe because Builder.CreateLoad always creates a bona fide
+  // LoadInst.
   LoadInst *LI = cast<LoadInst>(Builder.CreateLoad(Bitcast));
   propagateMetadata(LI, Chain);
   LI->setAlignment(Alignment);
 
   if (VecLoadTy) {
     SmallVector<Instruction *, 16> InstrsToErase;
 
     unsigned VecWidth = VecLoadTy->getNumElements();
     for (unsigned I = 0, E = Chain.size(); I != E; ++I) {
       for (auto Use : Chain[I]->users()) {
+        // All users of vector loads are ExtractElement instructions with
+        // constant indices, otherwise we would have bailed before now.
         Instruction *UI = cast<Instruction>(Use);
         unsigned Idx = cast<ConstantInt>(UI->getOperand(1))->getZExtValue();
         unsigned NewIdx = Idx + I * VecWidth;
         Value *V = Builder.CreateExtractElement(LI, Builder.getInt32(NewIdx));
-        Instruction *Extracted = cast<Instruction>(V);
-        if (Extracted->getType() != UI->getType())
-          Extracted = cast<Instruction>(
-              Builder.CreateBitCast(Extracted, UI->getType()));
+        if (V->getType() != UI->getType())
+          V = Builder.CreateBitCast(V, UI->getType());
 
         // Replace the old instruction.
-        UI->replaceAllUsesWith(Extracted);
+        UI->replaceAllUsesWith(V);
         InstrsToErase.push_back(UI);
       }
     }
 
     // Bitcast might not be an Instruction, if the value being loaded is a
     // constant.  In that case, no need to reorder anything.
     if (Instruction *BitcastInst = dyn_cast<Instruction>(Bitcast))
       reorder(BitcastInst);
 
     for (auto I : InstrsToErase)
       I->eraseFromParent();
   } else {
     for (unsigned I = 0, E = Chain.size(); I != E; ++I) {
       Value *V = Builder.CreateExtractElement(LI, Builder.getInt32(I));
-      Instruction *Extracted = cast<Instruction>(V);
-      Instruction *UI = cast<Instruction>(Chain[I]);
-      if (Extracted->getType() != UI->getType()) {
-        Extracted = cast<Instruction>(
-            Builder.CreateBitOrPointerCast(Extracted, UI->getType()));
+      Value *CV = Chain[I];
+      if (V->getType() != CV->getType()) {
+        V = Builder.CreateBitOrPointerCast(V, CV->getType());
       }
 
       // Replace the old instruction.
-      UI->replaceAllUsesWith(Extracted);
+      CV->replaceAllUsesWith(V);
     }
 
     if (Instruction *BitcastInst = dyn_cast<Instruction>(Bitcast))
       reorder(BitcastInst);
   }
 
   eraseInstructions(Chain);