[LSV] Don't assume that bitcast ops are Instructions.

[jlebar]

Summary

When we ask the builder to create a bitcast on a constant, we get back a
constant, not an instruction.

[asbirlea]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

972 ↗

(On Diff #65770)

Looking back at this, why do we need a small vector for instructions to reorder and not just one instruction (which now may or may not exist)?

975 ↗

(On Diff #65770)

Can you rename I to something else? It's being used right below as an unsigned iterator, and while the two scopes don't overlap, it can be confusing.

1003 ↗

(On Diff #65770)

Same as above.

[jlebar]

Address review comments.

[jlebar]

There are other places where we're assuming that Builder gives us an instruction when it doesn't, but I'd like to deal with those in a separate patch.

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

972 ↗

(On Diff #65770)

lol, good point. Fixed.

[asbirlea]

LGTM.

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

Value *Bitcast =
       Builder.CreateBitCast(L0->getPointerOperand(), VecTy->getPointerTo(AS));
 
   LoadInst *LI = cast<LoadInst>(Builder.CreateLoad(Bitcast));
   propagateMetadata(LI, Chain);
   LI->setAlignment(Alignment);
 
   if (VecLoadTy) {
     SmallVector<Instruction *, 16> InstrsToErase;
-    SmallVector<Instruction *, 16> InstrsToReorder;
-    InstrsToReorder.push_back(cast<Instruction>(Bitcast));
 
     unsigned VecWidth = VecLoadTy->getNumElements();
     for (unsigned I = 0, E = Chain.size(); I != E; ++I) {
       for (auto Use : Chain[I]->users()) {
         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()));
 
         // Replace the old instruction.
         UI->replaceAllUsesWith(Extracted);
         InstrsToErase.push_back(UI);
       }
     }
 
-    for (Instruction *ModUser : InstrsToReorder)
-      reorder(ModUser);
+    // 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 {
-    SmallVector<Instruction *, 16> InstrsToReorder;
-    InstrsToReorder.push_back(cast<Instruction>(Bitcast));
-
     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()));
       }
 
       // Replace the old instruction.
       UI->replaceAllUsesWith(Extracted);
     }
 
-    for (Instruction *ModUser : InstrsToReorder)
-      reorder(ModUser);
+    if (Instruction *BitcastInst = dyn_cast<Instruction>(Bitcast))
+      reorder(BitcastInst);
   }
 
   eraseInstructions(Chain);
 
   ++NumVectorInstructions;
   NumScalarsVectorized += Chain.size();
   return true;
 }

llvm/trunk/test/Transforms/LoadStoreVectorizer/NVPTX/non-instr-bitcast.ll

+; RUN: opt -mtriple=nvptx64-nvidia-cuda -load-store-vectorizer -S -o - %s | FileCheck %s
+
+; Load from a constant.  This can be vectorized, but shouldn't crash us.
+
+@global = internal addrspace(1) constant [4 x float] [float 0xBF71111120000000, float 0x3F70410420000000, float 0xBF81111120000000, float 0x3FB5555560000000], align 4
+
+define void @foo() {
+  ; CHECK: load <4 x float>
+  %a = load float, float addrspace(1)* getelementptr inbounds ([4 x float], [4 x float] addrspace(1)* @global, i64 0, i64 0), align 4
+  %b = load float, float addrspace(1)* getelementptr inbounds ([4 x float], [4 x float] addrspace(1)* @global, i64 0, i64 1), align 4
+  %c = load float, float addrspace(1)* getelementptr inbounds ([4 x float], [4 x float] addrspace(1)* @global, i64 0, i64 2), align 4
+  %d = load float, float addrspace(1)* getelementptr inbounds ([4 x float], [4 x float] addrspace(1)* @global, i64 0, i64 3), align 4
+  ret void
+}