<p dir="ltr">lgtm, full featured and implemented cleanly.</p>
<div class="gmail_quote">在 2013-5-29 上午1:59，"Redmond, Paul" <<a href="mailto:paul.redmond@intel.com">paul.redmond@intel.com</a>>写道：<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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><mailto:<a href="mailto:paul.redmond@intel.com">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><mailto:<a href="mailto:paul.redmond@intel.com">paul.redmond@intel.com</a>><mailto:<a href="mailto:paul.redmond@intel.com">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><mailto:<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><mailto:<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>
<br>
<Mail Attachment><br>
<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></blockquote></div>