<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 10, 2014 at 6:41 AM, Arnold Schwaighofer <span dir="ltr"><<a href="mailto:aschwaighofer@apple.com" target="_blank">aschwaighofer@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: arnolds<br>
Date: Thu Apr 10 08:41:35 2014<br>
New Revision: 205965<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=205965&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=205965&view=rev</a><br>
Log:<br>
Reapply "SLPVectorizer: Ignore users that are insertelements we can reschedule them"<br>
<br>
This commit reapplies 205018. After 205855 we should correctly vectorize<br>
intrinsics.<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=205965&r1=205964&r2=205965&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=205965&r1=205964&r2=205965&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Thu Apr 10 08:41:35 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 and is possibly<br>
- /// used by a reduction of \p RdxOps.<br>
- void buildTree(ArrayRef<Value *> Roots, ValueSet *RdxOps = 0);<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>
<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>
- /// Reduction operators.<br>
- ValueSet *RdxOps;<br>
+ /// List of users to ignore during scheduling and that don't need extracting.<br>
+ ArrayRef<Value *> UserIgnoreList;<br>
<br>
// Analysis and block reference.<br>
Function *F;<br>
@@ -543,9 +543,10 @@ private:<br>
IRBuilder<> Builder;<br>
};<br>
<br>
-void BoUpSLP::buildTree(ArrayRef<Value *> Roots, ValueSet *Rdx) {<br>
+void BoUpSLP::buildTree(ArrayRef<Value *> Roots,<br>
+ ArrayRef<Value *> UserIgnoreLst) {<br>
deleteTree();<br>
- RdxOps = Rdx;<br>
+ UserIgnoreList = UserIgnoreLst;<br>
if (!getSameType(Roots))<br>
return;<br>
buildTree_rec(Roots, 0);<br>
@@ -577,8 +578,9 @@ void BoUpSLP::buildTree(ArrayRef<Value *<br>
if (!UserInst)<br>
continue;<br>
<br>
- // Ignore uses that are part of the reduction.<br>
- if (Rdx && std::find(Rdx->begin(), Rdx->end(), UserInst) != Rdx->end())<br>
+ // Ignore users in the user ignore list.<br>
+ if (std::find(UserIgnoreList.begin(), UserIgnoreList.end(), UserInst) !=<br>
+ UserIgnoreList.end())<br>
continue;<br>
<br>
DEBUG(dbgs() << "SLP: Need to extract:" << *U << " from lane " <<<br>
@@ -709,8 +711,9 @@ void BoUpSLP::buildTree_rec(ArrayRef<Val<br>
continue;<br>
}<br>
<br>
- // This user is part of the reduction.<br>
- if (RdxOps && RdxOps->count(UI))<br>
+ // Ignore users in the user ignore list.<br>
+ if (std::find(UserIgnoreList.begin(), UserIgnoreList.end(), UI) !=<br>
+ UserIgnoreList.end())<br>
continue;<br>
<br>
// Make sure that we can schedule this unknown user.<br>
@@ -1749,8 +1752,9 @@ Value *BoUpSLP::vectorizeTree() {<br>
DEBUG(dbgs() << "SLP: \tvalidating user:" << *U << ".\n");<br>
<br>
assert((ScalarToTreeEntry.count(U) ||<br>
- // It is legal to replace the reduction users by undef.<br>
- (RdxOps && RdxOps->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>
"Replacing out-of-tree value with undef");<br>
}<br>
#endif<br>
@@ -1954,8 +1958,11 @@ 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>
+ bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,<br>
+ ArrayRef<Value *> BuildVector = None);<br>
<br>
/// \brief Try to vectorize a chain that may start at the operands of \V;<br>
bool tryToVectorize(BinaryOperator *V, BoUpSLP &R);<br>
@@ -2128,7 +2135,8 @@ bool SLPVectorizer::tryToVectorizePair(V<br>
return tryToVectorizeList(VL, R);<br>
}<br>
<br>
-bool SLPVectorizer::tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R) {<br>
+bool SLPVectorizer::tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,<br>
+ ArrayRef<Value *> BuildVector) {<br>
if (VL.size() < 2)<br>
return false;<br>
<br>
@@ -2178,13 +2186,33 @@ bool SLPVectorizer::tryToVectorizeList(A<br>
<< "\n");<br>
ArrayRef<Value *> Ops = VL.slice(i, OpsWidth);<br>
<br>
- R.buildTree(Ops);<br>
+ ArrayRef<Value *> BuildVectorSlice;<br>
+ if (!BuildVector.empty())<br>
+ BuildVectorSlice = BuildVector.slice(i, OpsWidth);<br>
+<br>
+ R.buildTree(Ops, BuildVectorSlice);<br>
int Cost = R.getTreeCost();<br>
<br>
if (Cost < -SLPCostThreshold) {<br>
DEBUG(dbgs() << "SLP: Vectorizing list at cost:" << Cost << ".\n");<br>
- R.vectorizeTree();<br>
+ Value *VectorizedRoot = 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></blockquote><div><br></div><div>Hey Arnold, sorry for the thread necromancy here.</div><div><br></div><div>It turns out that this is causing PR19621, along with a decent number of other crashers which I didn't really associate with this revision (or the reduced test case in PR19621), but reverting this patch causes all of these crashers to go away. I can't really tell if its that this commit causes the SLP vectorizer to fire way more often and exposes other bugs, or it is actually a bug here. However, PR19621 is nicely reduced and clearly points at some aspect of this code being wrong. My theory for why this took so long to turn up is that with some of the very recent improvements to the SLP vectorizer, we're seeing it fire on a lot more code, and so finding new patterns that just happen to reduce to something pointing here.</div>
<div><br></div><div>I tried to work out exactly what this code *should* be doing when we get a constant vector back as the VectorizedRoot (in this PR, undef), but I couldn't make heads or tails of it. emitReduction() also seems to make some assumptions about the root being an instruction, but its a bit less clear there and I don't have a test case for that.</div>
<div><br></div><div>I'm going to revert this for now to at least let us make progress past PR19621 now that we have a reduced test case for you. Let me know if I can help in some way with debugging this or fixing it.</div>
<div><br></div><div>Sorry for any trouble,</div><div>-Chandler</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ 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>
@@ -2293,7 +2321,7 @@ static Value *createRdxShuffleMask(unsig<br>
/// *p =<br>
///<br>
class HorizontalReduction {<br>
- SmallPtrSet<Value *, 16> ReductionOps;<br>
+ SmallVector<Value *, 16> ReductionOps;<br>
SmallVector<Value *, 32> ReducedVals;<br>
<br>
BinaryOperator *ReductionRoot;<br>
@@ -2387,7 +2415,7 @@ public:<br>
// We need to be able to reassociate the adds.<br>
if (!TreeN->isAssociative())<br>
return false;<br>
- ReductionOps.insert(TreeN);<br>
+ ReductionOps.push_back(TreeN);<br>
}<br>
// Retract.<br>
Stack.pop_back();<br>
@@ -2424,7 +2452,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>
@@ -2543,13 +2571,16 @@ private:<br>
///<br>
/// Returns true if it matches<br>
///<br>
-static bool findBuildVector(InsertElementInst *IE,<br>
- SmallVectorImpl<Value *> &Ops) {<br>
- if (!isa<UndefValue>(IE->getOperand(0)))<br>
+static bool findBuildVector(InsertElementInst *FirstInsertElem,<br>
+ SmallVectorImpl<Value *> &BuildVector,<br>
+ SmallVectorImpl<Value *> &BuildVectorOpds) {<br>
+ if (!isa<UndefValue>(FirstInsertElem->getOperand(0)))<br>
return false;<br>
<br>
+ InsertElementInst *IE = FirstInsertElem;<br>
while (true) {<br>
- Ops.push_back(IE->getOperand(1));<br>
+ BuildVector.push_back(IE);<br>
+ BuildVectorOpds.push_back(IE->getOperand(1));<br>
<br>
if (IE->use_empty())<br>
return false;<br>
@@ -2719,12 +2750,16 @@ bool SLPVectorizer::vectorizeChainsInBlo<br>
}<br>
<br>
// Try to vectorize trees that start at insertelement instructions.<br>
- if (InsertElementInst *IE = dyn_cast<InsertElementInst>(it)) {<br>
- SmallVector<Value *, 8> Ops;<br>
- if (!findBuildVector(IE, Ops))<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>
continue;<br>
<br>
- if (tryToVectorizeList(Ops, R)) {<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>
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=205965&r1=205964&r2=205965&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll?rev=205965&r1=205964&r2=205965&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 Apr 10 08:41:35 2014<br>
@@ -195,6 +195,30 @@ 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>
</blockquote></div><br></div></div>