<p dir="ltr">My suspicion is that after debugging, Arnold will end up with an even cleaner testcase. I figured he would check in whatever he ended up with.</p>
<div class="gmail_quote">On May 1, 2014 7:01 AM, "Rafael Espíndola" <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
what about checking in the testcase in PR19621?<br>
<br>
On 1 May 2014 07:24, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> Author: chandlerc<br>
> Date: Thu May 1 06:24:11 2014<br>
> New Revision: 207746<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=207746&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=207746&view=rev</a><br>
> Log:<br>
> Revert r205965, which essentially reverts r205018 for the second time.<br>
> =[<br>
><br>
> Turns out that this was the root cause of PR19621. We found a crasher<br>
> only recently (likely due to improvements elsewhere in the SLP<br>
> vectorizer) but the reduced test case failed all the way back to here.<br>
> I've confirmed that reverting this patch both fixes the reduced test<br>
> case in PR19621 and the actual source file that led to it, so it seems<br>
> to really be rooted here. I've replied to the commit thread with<br>
> discussion of my (feeble) attempts to debug this. Didn't make it very<br>
> far, so reverting now that we have a good test case so that things can<br>
> get back to healthy while the debugging carries on.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll<br>
><br>
> Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=207746&r1=207745&r2=207746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=207746&r1=207745&r2=207746&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Thu May 1 06:24:11 2014<br>
> @@ -366,13 +366,13 @@ public:<br>
> /// A negative number means that this is profitable.<br>
> int getTreeCost();<br>
><br>
> - /// Construct a vectorizable tree that starts at \p Roots, ignoring users for<br>
> - /// the purpose of scheduling and extraction in the \p UserIgnoreLst.<br>
> - void buildTree(ArrayRef<Value *> Roots,<br>
> - ArrayRef<Value *> UserIgnoreLst = None);<br>
> + /// Construct a vectorizable tree that starts at \p Roots and is possibly<br>
> + /// used by a reduction of \p RdxOps.<br>
> + void buildTree(ArrayRef<Value *> Roots, ValueSet *RdxOps = 0);<br>
><br>
> /// Clear the internal data structures that are created by 'buildTree'.<br>
> void deleteTree() {<br>
> + RdxOps = 0;<br>
> VectorizableTree.clear();<br>
> ScalarToTreeEntry.clear();<br>
> MustGather.clear();<br>
> @@ -528,8 +528,8 @@ private:<br>
> /// Numbers instructions in different blocks.<br>
> DenseMap<BasicBlock *, BlockNumbering> BlocksNumbers;<br>
><br>
> - /// List of users to ignore during scheduling and that don't need extracting.<br>
> - ArrayRef<Value *> UserIgnoreList;<br>
> + /// Reduction operators.<br>
> + ValueSet *RdxOps;<br>
><br>
> // Analysis and block reference.<br>
> Function *F;<br>
> @@ -543,10 +543,9 @@ private:<br>
> IRBuilder<> Builder;<br>
> };<br>
><br>
> -void BoUpSLP::buildTree(ArrayRef<Value *> Roots,<br>
> - ArrayRef<Value *> UserIgnoreLst) {<br>
> +void BoUpSLP::buildTree(ArrayRef<Value *> Roots, ValueSet *Rdx) {<br>
> deleteTree();<br>
> - UserIgnoreList = UserIgnoreLst;<br>
> + RdxOps = Rdx;<br>
> if (!getSameType(Roots))<br>
> return;<br>
> buildTree_rec(Roots, 0);<br>
> @@ -578,9 +577,8 @@ void BoUpSLP::buildTree(ArrayRef<Value *<br>
> if (!UserInst)<br>
> continue;<br>
><br>
> - // Ignore users in the user ignore list.<br>
> - if (std::find(UserIgnoreList.begin(), UserIgnoreList.end(), UserInst) !=<br>
> - UserIgnoreList.end())<br>
> + // Ignore uses that are part of the reduction.<br>
> + if (Rdx && std::find(Rdx->begin(), Rdx->end(), UserInst) != Rdx->end())<br>
> continue;<br>
><br>
> DEBUG(dbgs() << "SLP: Need to extract:" << *U << " from lane " <<<br>
> @@ -711,9 +709,8 @@ void BoUpSLP::buildTree_rec(ArrayRef<Val<br>
> continue;<br>
> }<br>
><br>
> - // Ignore users in the user ignore list.<br>
> - if (std::find(UserIgnoreList.begin(), UserIgnoreList.end(), UI) !=<br>
> - UserIgnoreList.end())<br>
> + // This user is part of the reduction.<br>
> + if (RdxOps && RdxOps->count(UI))<br>
> continue;<br>
><br>
> // Make sure that we can schedule this unknown user.<br>
> @@ -1752,9 +1749,8 @@ Value *BoUpSLP::vectorizeTree() {<br>
> DEBUG(dbgs() << "SLP: \tvalidating user:" << *U << ".\n");<br>
><br>
> assert((ScalarToTreeEntry.count(U) ||<br>
> - // It is legal to replace users in the ignorelist by undef.<br>
> - (std::find(UserIgnoreList.begin(), UserIgnoreList.end(), U) !=<br>
> - UserIgnoreList.end())) &&<br>
> + // It is legal to replace the reduction users by undef.<br>
> + (RdxOps && RdxOps->count(U))) &&<br>
> "Replacing out-of-tree value with undef");<br>
> }<br>
> #endif<br>
> @@ -1958,11 +1954,8 @@ private:<br>
> bool tryToVectorizePair(Value *A, Value *B, BoUpSLP &R);<br>
><br>
> /// \brief Try to vectorize a list of operands.<br>
> - /// \@param BuildVector A list of users to ignore for the purpose of<br>
> - /// scheduling and that don't need extracting.<br>
> /// \returns true if a value was vectorized.<br>
> - bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,<br>
> - ArrayRef<Value *> BuildVector = None);<br>
> + bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R);<br>
><br>
> /// \brief Try to vectorize a chain that may start at the operands of \V;<br>
> bool tryToVectorize(BinaryOperator *V, BoUpSLP &R);<br>
> @@ -2135,8 +2128,7 @@ bool SLPVectorizer::tryToVectorizePair(V<br>
> return tryToVectorizeList(VL, R);<br>
> }<br>
><br>
> -bool SLPVectorizer::tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,<br>
> - ArrayRef<Value *> BuildVector) {<br>
> +bool SLPVectorizer::tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R) {<br>
> if (VL.size() < 2)<br>
> return false;<br>
><br>
> @@ -2186,33 +2178,13 @@ bool SLPVectorizer::tryToVectorizeList(A<br>
> << "\n");<br>
> ArrayRef<Value *> Ops = VL.slice(i, OpsWidth);<br>
><br>
> - ArrayRef<Value *> BuildVectorSlice;<br>
> - if (!BuildVector.empty())<br>
> - BuildVectorSlice = BuildVector.slice(i, OpsWidth);<br>
> -<br>
> - R.buildTree(Ops, BuildVectorSlice);<br>
> + R.buildTree(Ops);<br>
> int Cost = R.getTreeCost();<br>
><br>
> if (Cost < -SLPCostThreshold) {<br>
> DEBUG(dbgs() << "SLP: Vectorizing list at cost:" << Cost << ".\n");<br>
> - Value *VectorizedRoot = R.vectorizeTree();<br>
> + R.vectorizeTree();<br>
><br>
> - // Reconstruct the build vector by extracting the vectorized root. This<br>
> - // way we handle the case where some elements of the vector are undefined.<br>
> - // (return (inserelt <4 xi32> (insertelt undef (opd0) 0) (opd1) 2))<br>
> - if (!BuildVectorSlice.empty()) {<br>
> - Instruction *InsertAfter = cast<Instruction>(VectorizedRoot);<br>
> - for (auto &V : BuildVectorSlice) {<br>
> - InsertElementInst *IE = cast<InsertElementInst>(V);<br>
> - IRBuilder<> Builder(++BasicBlock::iterator(InsertAfter));<br>
> - Instruction *Extract = cast<Instruction>(<br>
> - Builder.CreateExtractElement(VectorizedRoot, IE->getOperand(2)));<br>
> - IE->setOperand(1, Extract);<br>
> - IE->removeFromParent();<br>
> - IE->insertAfter(Extract);<br>
> - InsertAfter = IE;<br>
> - }<br>
> - }<br>
> // Move to the next bundle.<br>
> i += VF - 1;<br>
> Changed = true;<br>
> @@ -2321,7 +2293,7 @@ static Value *createRdxShuffleMask(unsig<br>
> /// *p =<br>
> ///<br>
> class HorizontalReduction {<br>
> - SmallVector<Value *, 16> ReductionOps;<br>
> + SmallPtrSet<Value *, 16> ReductionOps;<br>
> SmallVector<Value *, 32> ReducedVals;<br>
><br>
> BinaryOperator *ReductionRoot;<br>
> @@ -2415,7 +2387,7 @@ public:<br>
> // We need to be able to reassociate the adds.<br>
> if (!TreeN->isAssociative())<br>
> return false;<br>
> - ReductionOps.push_back(TreeN);<br>
> + ReductionOps.insert(TreeN);<br>
> }<br>
> // Retract.<br>
> Stack.pop_back();<br>
> @@ -2452,7 +2424,7 @@ public:<br>
><br>
> for (; i < NumReducedVals - ReduxWidth + 1; i += ReduxWidth) {<br>
> ArrayRef<Value *> ValsToReduce(&ReducedVals[i], ReduxWidth);<br>
> - V.buildTree(ValsToReduce, ReductionOps);<br>
> + V.buildTree(ValsToReduce, &ReductionOps);<br>
><br>
> // Estimate cost.<br>
> int Cost = V.getTreeCost() + getReductionCost(TTI, ReducedVals[i]);<br>
> @@ -2571,16 +2543,13 @@ private:<br>
> ///<br>
> /// Returns true if it matches<br>
> ///<br>
> -static bool findBuildVector(InsertElementInst *FirstInsertElem,<br>
> - SmallVectorImpl<Value *> &BuildVector,<br>
> - SmallVectorImpl<Value *> &BuildVectorOpds) {<br>
> - if (!isa<UndefValue>(FirstInsertElem->getOperand(0)))<br>
> +static bool findBuildVector(InsertElementInst *IE,<br>
> + SmallVectorImpl<Value *> &Ops) {<br>
> + if (!isa<UndefValue>(IE->getOperand(0)))<br>
> return false;<br>
><br>
> - InsertElementInst *IE = FirstInsertElem;<br>
> while (true) {<br>
> - BuildVector.push_back(IE);<br>
> - BuildVectorOpds.push_back(IE->getOperand(1));<br>
> + Ops.push_back(IE->getOperand(1));<br>
><br>
> if (IE->use_empty())<br>
> return false;<br>
> @@ -2751,16 +2720,12 @@ bool SLPVectorizer::vectorizeChainsInBlo<br>
> }<br>
><br>
> // Try to vectorize trees that start at insertelement instructions.<br>
> - if (InsertElementInst *FirstInsertElem = dyn_cast<InsertElementInst>(it)) {<br>
> - SmallVector<Value *, 16> BuildVector;<br>
> - SmallVector<Value *, 16> BuildVectorOpds;<br>
> - if (!findBuildVector(FirstInsertElem, BuildVector, BuildVectorOpds))<br>
> + if (InsertElementInst *IE = dyn_cast<InsertElementInst>(it)) {<br>
> + SmallVector<Value *, 8> Ops;<br>
> + if (!findBuildVector(IE, Ops))<br>
> continue;<br>
><br>
> - // Vectorize starting with the build vector operands ignoring the<br>
> - // BuildVector instructions for the purpose of scheduling and user<br>
> - // extraction.<br>
> - if (tryToVectorizeList(BuildVectorOpds, R, BuildVector)) {<br>
> + if (tryToVectorizeList(Ops, R)) {<br>
> Changed = true;<br>
> it = BB->begin();<br>
> e = BB->end();<br>
><br>
> Modified: llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll?rev=207746&r1=207745&r2=207746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll?rev=207746&r1=207745&r2=207746&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll (original)<br>
> +++ llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll Thu May 1 06:24:11 2014<br>
> @@ -195,30 +195,6 @@ define <4 x float> @simple_select_partia<br>
> ret <4 x float> %rb<br>
> }<br>
><br>
> -; Make sure that vectorization happens even if insertelements operations<br>
> -; must be rescheduled. The case here is from compiling Julia.<br>
> -define <4 x float> @reschedule_extract(<4 x float> %a, <4 x float> %b) {<br>
> -; CHECK-LABEL: @reschedule_extract(<br>
> -; CHECK: %1 = fadd <4 x float> %a, %b<br>
> - %a0 = extractelement <4 x float> %a, i32 0<br>
> - %b0 = extractelement <4 x float> %b, i32 0<br>
> - %c0 = fadd float %a0, %b0<br>
> - %v0 = insertelement <4 x float> undef, float %c0, i32 0<br>
> - %a1 = extractelement <4 x float> %a, i32 1<br>
> - %b1 = extractelement <4 x float> %b, i32 1<br>
> - %c1 = fadd float %a1, %b1<br>
> - %v1 = insertelement <4 x float> %v0, float %c1, i32 1<br>
> - %a2 = extractelement <4 x float> %a, i32 2<br>
> - %b2 = extractelement <4 x float> %b, i32 2<br>
> - %c2 = fadd float %a2, %b2<br>
> - %v2 = insertelement <4 x float> %v1, float %c2, i32 2<br>
> - %a3 = extractelement <4 x float> %a, i32 3<br>
> - %b3 = extractelement <4 x float> %b, i32 3<br>
> - %c3 = fadd float %a3, %b3<br>
> - %v3 = insertelement <4 x float> %v2, float %c3, i32 3<br>
> - ret <4 x float> %v3<br>
> -}<br>
> -<br>
> ; Check that cost model for vectorization takes credit for<br>
> ; instructions that are erased.<br>
> define <4 x float> @take_credit(<4 x float> %a, <4 x float> %b) {<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>