<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;">HI Paul, <div><br></div><div>Thanks for working on this. This patch LGTM with one comment and a few nit picking. </div><div><br></div><div><div><div>+ LoopVectorizeHints Hints(L);</div><div>+</div><div>+ if (Hints.AlreadyVectorized) {</div><div>+ DEBUG(dbgs() << "LV: This loop was vectorized before\n");</div><div>+ return false;</div><div>+ }</div><div>+ if (Hints.Disable) {</div><div>+ DEBUG(dbgs() << "LV: Not vectorizing (disabled).\n");</div><div>+ return false;</div><div>+ }</div><div>+</div></div><div><br></div><div> Why do we need both “Disable Vectorization” and “Already Vectorized” ? I think that we can merge them into one name.</div><div><br></div><div><br></div><div><div>+MDNode *Loop::getLoopID() const {</div><div>+ MDNode *LoopID = 0;</div><div>+ if (isLoopSimplifyForm()) {</div><div>+ LoopID = getLoopLatch()->getTerminator()->getMetadata("llvm.loop");</div><div>+ } else {</div><div>+ // Go through each predecessor of the loop header and check the</div><div>+ // terminator for the metadata.</div><div>+ BasicBlock *H = getHeader();</div><div>+ for (block_iterator I = block_begin(), IE = block_end(); I != IE; ++I) {</div><div><br></div></div><div>Why are you not using the Latch block ? Is it because of rotated loops ?</div><div><br></div><div><div>+ getLoopLatch()->getTerminator()->setMetadata("llvm.loop", LoopID);</div></div><div><br></div><div>Can you save the constant “llvm.loop” in a static variable ?</div><div><br></div><div><div>+ /// Mark the loop L as already vectorized using the already_vectozed hint.</div><div>+ void setAlreadyVectorized(Loop *L) {</div><div>+ assert(!AlreadyVectorized);</div><div><br></div></div><div>Assertions should have a string to describe the error. </div><div><br></div><div><div>+ if (LoopID) {</div><div>+ for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i)</div><div>+ Vals.push_back(LoopID->getOperand(i));</div><div>+ }</div></div><div><br></div><div>No need for braces. </div><div><br></div><div><div>+ </div><div>+ // First operand should refer to the loop id itself.</div><div>+ assert(LoopID->getNumOperands() > 0);</div><div>+ assert(LoopID->getOperand(0) == LoopID);</div></div><div><br></div><div>Asserts need to have strings. </div><div><br></div><div><div>+ // Check string hint with one operand.</div><div>+ void getHint(StringRef Hint, Value *Arg) {</div><div>+ if (Hint == "width") {</div><div>+ const ConstantInt *C = dyn_cast<ConstantInt>(Arg);</div><div>+ Width = C ? C->getZExtValue() : Width;</div><div>+ } else if (Hint == "unroll") {</div><div>+ const ConstantInt *C = dyn_cast<ConstantInt>(Arg);</div><div>+ Unroll = C ? C->getZExtValue() : Unroll;</div><div>+ }</div><div>+ }</div><div>+};</div></div><div><br></div><div><br></div><div>Can you verify that the values of unroll and width are reasonable ? Such as power of two, smaller than 128, etc. </div><div><br></div><div><br></div><div>Did you decide to call it ‘llvm.vectorizer’ or ‘llvm.vectorize’ ? I am okay with either one. <br></div><div><br></div><div><br></div><div>Thanks,</div><div>Nadav</div><div><br><div><div>On May 27, 2013, at 12:42 PM, "Redmond, Paul" <<a href="mailto:paul.redmond@intel.com">paul.redmond@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: 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;">Attached is an updated patch that:<br><br>- documents llvm.vectorizer.* metadata<br>- documents llvm.loop and updates llvm.mem.parallel_loop_access<br>- moves loop vectorizer metadata handling into utility class LoopVectorizerHints<br>- moves getting/setting of llvm.vectorizer.already_vectorized into LoopVectorizerHints<br>- adds support for llvm.vectorizer.width, llvm.vectorizer.unroll, and llvm.vectorizer.disable<br> - llvm.vectorizer.force and llvm.vectorizer.max_iterations will be addressed later.<br>- updated tests<br><br>paul<br><br><br>On 2013-05-24, at 2:07 PM, Nadav Rotem wrote:<br><br>Hi Paul,<br><br>Thanks for working on this. Can you also refactor the code that sets metadata on vectorized loops (so that they are not re-vectorized) ?<br><br><br>-bool Loop::isAnnotatedParallel() const {<br>+MDNode *Loop::getLoopID() const {<br>+ MDNode *LoopID = 0;<br><br>Please add braces below.<br><br>+ if (isLoopSimplifyForm())<br>+ LoopID = getLoopLatch()->getTerminator()->getMetadata("llvm.loop");<br>+ else {<br>+ // Go through each predecessor of the loop header and check the<br>+ // terminator for the metadata.<br>+ BasicBlock *H<br><br><br>Please add a string for the asserts below.<br><br>+void Loop::setLoopID(MDNode *LoopID) const {<br>+ assert(LoopID);<br>+ assert(LoopID->getNumOperands() > 0);<br>+ assert(LoopID->getOperand(0) == LoopID);<br><br><br>Can you outline the code below to a function and use it around the code that checks for the command line flags ?<br><br>+ unsigned UserVF = VectorizationFactor;<br>+<br>+ // Check the loop metadata for vectorization hints.<br>+ if (const MDNode *LoopID = L->getLoopID()) {<br>+ assert(LoopID->getNumOperands() > 0);<br>+ assert(LoopID->getOperand(0) == LoopID);<br>+ for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {<br>+ const MDNode *MD = dyn_cast<MDNode>(LoopID->getOperand(i));<br>+ if (!MD)<br>+ continue;<br><br>Use early exits (continue) here:<br><br>+ if (MD->getNumOperands() == 2) {<br>+ if (const MDString *S = dyn_cast<MDString>(MD->getOperand(0))) {<br>+ if (S->getString() == "llvm.vectorization.vector_width") {<br>+ const ConstantInt *C = dyn_cast<ConstantInt>(MD->getOperand(1));<br>+ UserVF = C ? C->getZExtValue() : UserVF;<br><br><br>Because of this:<br><br>+ }<br>+ }<br>+ }<br>+ }<br>+ }<br>+<br><br><br>Thanks,<br>Nadav<br><br><br>On May 24, 2013, at 10:49 AM, "Redmond, Paul" <<a href="mailto:paul.redmond@intel.com">paul.redmond@intel.com</a><<a href="mailto:paul.redmond@intel.com">mailto:paul.redmond@intel.com</a>>> wrote:<br><br>Hi,<br><br>The attached patch modifies LoopVectorizer to recognize llvm.vectorization.vector_width metadata attached to llvm.loop metadata.<br><br>The llvm.loop.parallel metadata has been renamed to llvm.loop to be more generic. Loop::isAnnotatedParallel now looks for llvm.loop and associated llvm.mem.parallel_loop_access. If we decide that we still need an explicit llvm.loop.parallel metadata then it can be added as a child of llvm.loop.<br><br>Loop::setLoopID was added for symmetry and it is used in a forthcoming patch which preserves llvm.loop metadata in loop passes.<br><br>paul<br><br><Mail Attachment>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto:llvm-commits@cs.uiuc.edu</a>><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br><br><br><span><Mail Attachment></span></div></blockquote></div><br></div></div></body></html>