[PATCH] Fix inserting instructions before last in bundle.

[Nadav Rotem]

Hi Matt, 

Thanks for working on this. :)   The scalars are erased from the function so it does not matter. You changed the debug info location from the first scalar to the last scalar. I don’t think that it matters which one we use as long as we are consistent. I like the way that you refactored the code. 

Thanks,
Nadav

On Aug 26, 2013, at 11:48 AM, Matt Arsenault <Matthew.Arsenault at amd.com> wrote:

> Hi nadav,
> 
> The builder inserts from before the insert point,
> not after, so this would insert before the last
> instruction in the bundle instead of after it.
> 
> I'm not sure if this can actually be a problem with any of the current insertions.
> 
> http://llvm-reviews.chandlerc.com/D1518
> 
> Files:
>  lib/Transforms/Vectorize/SLPVectorizer.cpp
> 
> Index: lib/Transforms/Vectorize/SLPVectorizer.cpp
> ===================================================================
> --- lib/Transforms/Vectorize/SLPVectorizer.cpp
> +++ lib/Transforms/Vectorize/SLPVectorizer.cpp
> @@ -311,6 +311,10 @@
>   /// \returns the Instruction in the bundle \p VL.
>   Instruction *getLastInstruction(ArrayRef<Value *> VL);
> 
> +  /// \brief Set the Builder insert point to one after the last instruction in
> +  /// the bundle
> +  void setInsertPointAfterBundle(ArrayRef<Value *> VL);
> +
>   /// \returns a vector from a collection of scalars in \p VL.
>   Value *Gather(ArrayRef<Value *> VL, VectorType *Ty);
> 
> @@ -1068,6 +1072,15 @@
>   return I;
> }
> 
> +void BoUpSLP::setInsertPointAfterBundle(ArrayRef<Value *> VL) {
> +  Instruction *LastInst = getLastInstruction(VL);
> +  Builder.SetCurrentDebugLocation(LastInst->getDebugLoc());
> +
> +  BasicBlock::iterator NextInst = LastInst;
> +  ++NextInst;
> +  Builder.SetInsertPoint(cast<Instruction>(VL[0])->getParent(), NextInst);
> +}
> +
> Value *BoUpSLP::Gather(ArrayRef<Value *> VL, VectorType *Ty) {
>   Value *Vec = UndefValue::get(Ty);
>   // Generate the 'InsertElement' instruction.
> @@ -1141,10 +1154,7 @@
>   VectorType *VecTy = VectorType::get(ScalarTy, E->Scalars.size());
> 
>   if (E->NeedToGather) {
> -    BasicBlock::iterator NextInst = getLastInstruction(E->Scalars);
> -    ++NextInst;
> -    assert(NextInst != VL0->getParent()->end());
> -    Builder.SetInsertPoint(NextInst);
> +    setInsertPointAfterBundle(E->Scalars);
>     return Gather(E->Scalars, VecTy);
>   }
> 
> @@ -1212,8 +1222,7 @@
>       for (int i = 0, e = E->Scalars.size(); i < e; ++i)
>         INVL.push_back(cast<Instruction>(E->Scalars[i])->getOperand(0));
> 
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       Value *InVec = vectorizeTree(INVL);
> 
> @@ -1233,8 +1242,7 @@
>         RHSV.push_back(cast<Instruction>(E->Scalars[i])->getOperand(1));
>       }
> 
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       Value *L = vectorizeTree(LHSV);
>       Value *R = vectorizeTree(RHSV);
> @@ -1260,8 +1268,7 @@
>         FalseVec.push_back(cast<Instruction>(E->Scalars[i])->getOperand(2));
>       }
> 
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       Value *Cond = vectorizeTree(CondVec);
>       Value *True = vectorizeTree(TrueVec);
> @@ -1298,8 +1305,7 @@
>         RHSVL.push_back(cast<Instruction>(E->Scalars[i])->getOperand(1));
>       }
> 
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       Value *LHS = vectorizeTree(LHSVL);
>       Value *RHS = vectorizeTree(RHSVL);
> @@ -1319,8 +1325,7 @@
>     case Instruction::Load: {
>       // Loads are inserted at the head of the tree because we don't want to
>       // sink them all the way down past store instructions.
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       LoadInst *LI = cast<LoadInst>(VL0);
>       Value *VecPtr =
> @@ -1339,8 +1344,7 @@
>       for (int i = 0, e = E->Scalars.size(); i < e; ++i)
>         ValueOp.push_back(cast<StoreInst>(E->Scalars[i])->getValueOperand());
> 
> -      Builder.SetInsertPoint(getLastInstruction(E->Scalars));
> -      Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
> +      setInsertPointAfterBundle(E->Scalars);
> 
>       Value *VecValue = vectorizeTree(ValueOp);
>       Value *VecPtr =
> <D1518.1.patch>