<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>Hi Paul, </div><div><br></div><div>Thanks for working on this. </div><div><br></div><div>+ } else if (Hint == "unroll") {</div><div>+ if (isPowerOf2_32(Val) && Val <= MaxUnrollFactor)</div><div>+ Unroll = Val;</div><div>+ else</div><div>+ DEBUG(dbgs() << "LV: ignoring invalid unroll metadata.");</div><div>+ } else</div><div>+ DEBUG(dbgs() << "LV: ignoring unknown hint " << Hint);</div><div>+ }</div><div>+};</div></div><div><br></div><div>I think that you will have a problem with Release builds when the DEBUG macro is removed and the else becomes an invalid expression. You can solve this by adding braces. </div><div><br></div><div><div>+; RUN: opt < %s -loop-vectorize -force-vector-width=4 -force-vector-unroll=1 -dce -instcombine -S | FileCheck %s</div><div>+</div><div>+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"</div><div>+target triple = "x86_64-unknown-linux-gnu"</div><div><br></div></div><div><br></div><div>Do you need the command line flags to control vectorization ? I want the command line flag to override the metadata. This can help debuggability. </div><div><br></div><div>LGTM. </div><div><br></div><div>Thanks,</div><div>Nadav</div><div><br></div><div><br></div><br><div><div>On May 28, 2013, at 10:58 AM, "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;">Hi,<br><br>Attached is a new patch.<br><br>- use width=1 instead of already_vectorized/disabled<br>- removed disabled<br>- attempted to make documentation for llvm.vectorizer.width more clear<br>- check unroll and width are powers of two and unroll <= 16 and width <= 64<br>- addressed style issues<br><br>paul<br><br><br><br>On 2013-05-27, at 3:57 PM, Nadav Rotem wrote:<br><br>HI Paul,<br><br>Thanks for working on this. This patch LGTM with one comment and a few nit picking.<br><br>+ LoopVectorizeHints Hints(L);<br>+<br>+ if (Hints.AlreadyVectorized) {<br>+ DEBUG(dbgs() << "LV: This loop was vectorized before\n");<br>+ return false;<br>+ }<br>+ if (Hints.Disable) {<br>+ DEBUG(dbgs() << "LV: Not vectorizing (disabled).\n");<br>+ return false;<br>+ }<br>+<br><br>Why do we need both “Disable Vectorization” and “Already Vectorized” ? I think that we can merge them into one name.<br><br><br>+MDNode *Loop::getLoopID() const {<br>+ MDNode *LoopID = 0;<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 = getHeader();<br>+ for (block_iterator I = block_begin(), IE = block_end(); I != IE; ++I) {<br><br>Why are you not using the Latch block ? Is it because of rotated loops ?<br><br>+ getLoopLatch()->getTerminator()->setMetadata("llvm.loop", LoopID);<br><br>Can you save the constant “llvm.loop” in a static variable ?<br><br>+ /// Mark the loop L as already vectorized using the already_vectozed hint.<br>+ void setAlreadyVectorized(Loop *L) {<br>+ assert(!AlreadyVectorized);<br><br>Assertions should have a string to describe the error.<br><br>+ if (LoopID) {<br>+ for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i)<br>+ Vals.push_back(LoopID->getOperand(i));<br>+ }<br><br>No need for braces.<br><br>+<br>+ // First operand should refer to the loop id itself.<br>+ assert(LoopID->getNumOperands() > 0);<br>+ assert(LoopID->getOperand(0) == LoopID);<br><br>Asserts need to have strings.<br><br>+ // Check string hint with one operand.<br>+ void getHint(StringRef Hint, Value *Arg) {<br>+ if (Hint == "width") {<br>+ const ConstantInt *C = dyn_cast<ConstantInt>(Arg);<br>+ Width = C ? C->getZExtValue() : Width;<br>+ } else if (Hint == "unroll") {<br>+ const ConstantInt *C = dyn_cast<ConstantInt>(Arg);<br>+ Unroll = C ? C->getZExtValue() : Unroll;<br>+ }<br>+ }<br>+};<br><br><br>Can you verify that the values of unroll and width are reasonable ? Such as power of two, smaller than 128, etc.<br><br><br>Did you decide to call it ‘llvm.vectorizer’ or ‘llvm.vectorize’ ? I am okay with either one.<br><br><br>Thanks,<br>Nadav<br><br>On May 27, 2013, at 12:42 PM, "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>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>><<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>><<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><Mail Attachment><br><br><br><span><Mail Attachment></span></div></blockquote></div><br></body></html>