<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div style="margin: 0px; font-size: 13px; font-family: Menlo;">A few more comments:</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"> <span style="color: #3495af">Instruction</span>* x = BN.<span style="color: #3495af">getInstruction</span>(MyLastIndex);</div></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;">It should be “Instruction *x” and not ““Instruction* x”.</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><div style="margin: 0px;"> <span style="color: #0433ff">while</span> (IE-><span style="color: #3495af">getParent</span>()==BB) {</div><div><br></div><div>Spaces around “==“.</div><div><br></div></div><div><br></div><div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><span style="color: #0433ff">void</span> <span style="color: #3495af">BoUpSLP</span>::movePrematureInserts(<span style="color: #3495af">ArrayRef</span><<span style="color: #3495af">Value</span> *> VL, <span style="color: #3495af">InsertElementInst</span> *IE) {</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"><span style="color: #000000"> </span>Instruction<span style="color: #000000"> *VL0 = </span>cast<span style="color: #000000"><</span>Instruction<span style="color: #000000">>(VL[0]);</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"> <span style="color: #0433ff">int</span> MyLastIndex = <span style="color: #3495af">getLastIndex</span>(VL);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"><span style="color: #000000"> </span>BasicBlock<span style="color: #000000"> *BB = </span>cast<span style="color: #000000"><</span>Instruction<span style="color: #000000">>(VL0)-></span>getParent<span style="color: #000000">();</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"><span style="color: #000000"> </span>BlockNumbering<span style="color: #000000"> &BN = </span>BlocksNumbers<span style="color: #000000">[</span>BB<span style="color: #000000">];</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(180, 38, 26);"><span style="color: #000000"> </span><span style="color: #0433ff">DEBUG</span><span style="color: #000000">(</span><span style="color: #3495af">dbgs</span><span style="color: #000000">() << </span>"SLP: Moving premature inserts\n"<span style="color: #000000">);</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"> <span style="color: #3495af">Instruction</span>* x = BN.<span style="color: #3495af">getInstruction</span>(MyLastIndex);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;">What is X ? Is it the current insertion place? If so, it should be called ‘CurrentInsertLoc’. </div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"> x = IE;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"><span style="color: #000000"> IE = </span>dyn_cast<span style="color: #000000"><</span>InsertElementInst<span style="color: #000000">>(IE-></span>user_back<span style="color: #000000">());</span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(52, 149, 175);"><span style="color: #000000"><br></span></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;">What happens if IE has more than one user?</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><div style="margin: 0px;"> <span style="color: #0433ff">bool</span> tryToVectorizeList(<span style="color: #3495af">ArrayRef</span><<span style="color: #3495af">Value</span> *> VL, <span style="color: #3495af">BoUpSLP</span> &R, </div><div style="margin: 0px;"> <span style="color: #3495af">InsertElementInst</span> *IE = 0, </div><div style="margin: 0px;"> <span style="color: #3495af">BoUpSLP</span>::<span style="color: #3495af">ValueSet</span> *Inserts = 0);</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">The IE is not a pointer to an Insertion instruction, it is really the first Insert in the build_vector sequence. You should name it “BuildVectorStart” or something to indicate that this is not a single IE instruction. </div><div style="margin: 0px;"><br></div></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><div style="margin: 0px;"><span style="color: #0433ff">static</span> <span style="color: #0433ff">bool</span> findBuildVector(<span style="color: #3495af">InsertElementInst</span> *IE,</div><div style="margin: 0px;"> <span style="color: #3495af">SmallVectorImpl</span><<span style="color: #3495af">Value</span> *> &Ops,</div><div style="margin: 0px;"> <span style="color: #3495af">BoUpSLP</span>::<span style="color: #3495af">ValueSet</span> &Inserts) {</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">findBuildVector is a really odd function. It returns both the inserts that construct the vector AND the members of the vector? Also, what is IE? what is Ops and what is Inserts?</div><div style="margin: 0px;"><br></div></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;"><br></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;">-Nadav</div><div><br></div><div><br></div><br><div><div>On Mar 27, 2014, at 9:27 AM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br> Hi Arch,<br><br> I have a few comments below.<br><br> /// \brief Move InsertElement instructions with indices preceding<br> /// LastIndex. \p IE is the root of a chain identified by findBuildVector.<br> void movePrematureInserts(ArrayRef<Value *> VL, InsertElementInst *IE);<br><br> Please name this method “sinkInstructions”, or something similar that indicates that you are pushing the instructions down. The param IE should be named “location” or something that indicates that this is the destination of the move.<br><br><br> void BoUpSLP::buildTree(ArrayRef<Value *> Roots, ValueSet *Rdx,<br> bool RdxFreeExtract) {<br> assert(!RdxFreeExtract || Rdx);<br><br> Please add a message to the assertion that explains what went wrong.<br><br><br> // Ignore uses that are part of the reduction that do not need extracts.<br> if (RdxFreeExtract)<br> if (std::find(Rdx->begin(), Rdx->end(), UserInst) != Rdx->end())<br> continue;<br><br> You can && the conditions.<br><br><br> void BoUpSLP::movePrematureInserts(ArrayRef<Value *> VL, InsertElementInst *IE) {<br> Instruction *VL0 = cast<Instruction>(VL[0]);<br> int MyLastIndex = getLastIndex(VL);<br> BasicBlock *BB = cast<Instruction>(VL0)->getParent();<br> BlockNumbering &BN = BlocksNumbers[BB];<br> DEBUG(dbgs() << "SLP: Moving premature inserts\n”);<br><br> period at the end of the comment.<br><br><br> Instruction* x = BN.getInstruction(MyLastIndex);<br> while (IE->getParent()==BB) {<br> int UserIndex = BN.getIndex(IE);<br> if (UserIndex >= MyLastIndex) {<br> // Walked past transformed region<br><br><br> period at the end of the comment.<br><br> break;<br> }<br><br> IE->removeFromParent();<br> IE->insertAfter(x);<br><br><br> IE->moveBefore(x)<br><br><br> DEBUG(dbgs() << "SLP: Rescheduled: " << *IE << ".\n");<br> x = IE;<br> IE = dyn_cast<InsertElementInst>(IE->user_back());<br> if (!IE)<br> break;<br> }<br> }<br><br><br> /// \brief Try to vectorize a list of operands.<br> /// \returns true if a value was vectorized.<br> bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,<br> InsertElementInst *IE = 0,<br> BoUpSLP::ValueSet *Inserts = 0);<br><br><br> Please comment the argument names and give them meaningful names.<br><br><br> for (; i < NumReducedVals - ReduxWidth + 1; i += ReduxWidth) {<br> ArrayRef<Value *> ValsToReduce(&ReducedVals[i], ReduxWidth);<br> V.buildTree(ValsToReduce, &ReductionOps, true);<br><br> Please add a comment that explains why you are passing ’true’.<br><br><br><br> Thanks,<br> Nadav<br><br><a href="http://llvm-reviews.chandlerc.com/D3143">http://llvm-reviews.chandlerc.com/D3143</a></div></blockquote></div><br></body></html>