[PATCH] support llvm.vectorization.vector_width metadata

[Nadav Rotem]

HI Paul, 

Thanks for working on this. This patch LGTM with one comment and a few nit picking. 

+    LoopVectorizeHints Hints(L);
+
+    if (Hints.AlreadyVectorized) {
+      DEBUG(dbgs() << "LV: This loop was vectorized before\n");
+      return false;
+    }
+    if (Hints.Disable) {
+      DEBUG(dbgs() << "LV: Not vectorizing (disabled).\n");
+      return false;
+    }
+

 Why do we need both “Disable Vectorization” and “Already Vectorized” ? I think that we can merge them into one name.


+MDNode *Loop::getLoopID() const {
+  MDNode *LoopID = 0;
+  if (isLoopSimplifyForm()) {
+    LoopID = getLoopLatch()->getTerminator()->getMetadata("llvm.loop");
+  } else {
+    // Go through each predecessor of the loop header and check the
+    // terminator for the metadata.
+    BasicBlock *H = getHeader();
+    for (block_iterator I = block_begin(), IE = block_end(); I != IE; ++I) {

Why are you not using the Latch block ? Is it because of rotated loops ?

+    getLoopLatch()->getTerminator()->setMetadata("llvm.loop", LoopID);

Can you save the constant “llvm.loop” in a static variable ?

+  /// Mark the loop L as already vectorized using the already_vectozed hint.
+  void setAlreadyVectorized(Loop *L) {
+    assert(!AlreadyVectorized);

Assertions should have a string to describe the error. 

+    if (LoopID) {
+      for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i)
+        Vals.push_back(LoopID->getOperand(i));
+    }

No need for braces. 

+  
+    // First operand should refer to the loop id itself.
+    assert(LoopID->getNumOperands() > 0);
+    assert(LoopID->getOperand(0) == LoopID);

Asserts need to have strings. 

+  // Check string hint with one operand.
+  void getHint(StringRef Hint, Value *Arg) {
+    if (Hint == "width") {
+      const ConstantInt *C = dyn_cast<ConstantInt>(Arg);
+      Width = C ? C->getZExtValue() : Width;
+    } else if (Hint == "unroll") {
+      const ConstantInt *C = dyn_cast<ConstantInt>(Arg);
+      Unroll = C ? C->getZExtValue() : Unroll;
+    }
+  }
+};


Can you verify that the values of unroll and width are reasonable ? Such as power of two, smaller than 128, etc. 


Did you decide to call it ‘llvm.vectorizer’ or ‘llvm.vectorize’ ? I am okay with either one. 


Thanks,
Nadav

On May 27, 2013, at 12:42 PM, "Redmond, Paul" <paul.redmond at intel.com> wrote:

> Attached is an updated patch that:
> 
> - documents llvm.vectorizer.* metadata
> - documents llvm.loop and updates llvm.mem.parallel_loop_access
> - moves loop vectorizer metadata handling into utility class LoopVectorizerHints
> - moves getting/setting of llvm.vectorizer.already_vectorized into LoopVectorizerHints
> - adds support for llvm.vectorizer.width, llvm.vectorizer.unroll, and llvm.vectorizer.disable
>  - llvm.vectorizer.force and llvm.vectorizer.max_iterations will be addressed later.
> - updated tests
> 
> paul
> 
> 
> On 2013-05-24, at 2:07 PM, Nadav Rotem wrote:
> 
> Hi Paul,
> 
> Thanks for working on this.  Can you also refactor the code that sets metadata on vectorized loops (so that they are not re-vectorized) ?
> 
> 
> -bool Loop::isAnnotatedParallel() const {
> +MDNode *Loop::getLoopID() const {
> +  MDNode *LoopID = 0;
> 
> Please add braces below.
> 
> +  if (isLoopSimplifyForm())
> +    LoopID = getLoopLatch()->getTerminator()->getMetadata("llvm.loop");
> +  else {
> +    // Go through each predecessor of the loop header and check the
> +    // terminator for the metadata.
> +    BasicBlock *H
> 
> 
> Please add a string for the asserts below.
> 
> +void Loop::setLoopID(MDNode *LoopID) const {
> +  assert(LoopID);
> +  assert(LoopID->getNumOperands() > 0);
> +  assert(LoopID->getOperand(0) == LoopID);
> 
> 
> Can you outline the code below to a function and use it around the code that checks for the command line flags ?
> 
> +    unsigned UserVF = VectorizationFactor;
> +
> +    // Check the loop metadata for vectorization hints.
> +    if (const MDNode *LoopID = L->getLoopID()) {
> +      assert(LoopID->getNumOperands() > 0);
> +      assert(LoopID->getOperand(0) == LoopID);
> +      for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
> +        const MDNode *MD = dyn_cast<MDNode>(LoopID->getOperand(i));
> +        if (!MD)
> +          continue;
> 
> Use early exits (continue) here:
> 
> +        if (MD->getNumOperands() == 2) {
> +          if (const MDString *S = dyn_cast<MDString>(MD->getOperand(0))) {
> +            if (S->getString() == "llvm.vectorization.vector_width") {
> +              const ConstantInt *C = dyn_cast<ConstantInt>(MD->getOperand(1));
> +              UserVF = C ? C->getZExtValue() : UserVF;
> 
> 
> Because of this:
> 
> +            }
> +          }
> +        }
> +      }
> +    }
> +
> 
> 
> Thanks,
> Nadav
> 
> 
> On May 24, 2013, at 10:49 AM, "Redmond, Paul" <paul.redmond at intel.com<mailto:paul.redmond at intel.com>> wrote:
> 
> Hi,
> 
> The attached patch modifies LoopVectorizer to recognize llvm.vectorization.vector_width metadata attached to llvm.loop metadata.
> 
> 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.
> 
> Loop::setLoopID was added for symmetry and it is used in a forthcoming patch which preserves llvm.loop metadata in loop passes.
> 
> paul
> 
> <Mail Attachment>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> <Mail Attachment>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130527/0db9fdb1/attachment.html>