[PATCH] support llvm.vectorization.vector_width metadata
Nadav Rotem
nrotem at apple.com
Tue May 28 11:07:19 PDT 2013
Hi Paul,
Thanks for working on this.
+ } else if (Hint == "unroll") {
+ if (isPowerOf2_32(Val) && Val <= MaxUnrollFactor)
+ Unroll = Val;
+ else
+ DEBUG(dbgs() << "LV: ignoring invalid unroll metadata.");
+ } else
+ DEBUG(dbgs() << "LV: ignoring unknown hint " << Hint);
+ }
+};
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.
+; RUN: opt < %s -loop-vectorize -force-vector-width=4 -force-vector-unroll=1 -dce -instcombine -S | FileCheck %s
+
+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"
+target triple = "x86_64-unknown-linux-gnu"
Do you need the command line flags to control vectorization ? I want the command line flag to override the metadata. This can help debuggability.
LGTM.
Thanks,
Nadav
On May 28, 2013, at 10:58 AM, "Redmond, Paul" <paul.redmond at intel.com> wrote:
> Hi,
>
> Attached is a new patch.
>
> - use width=1 instead of already_vectorized/disabled
> - removed disabled
> - attempted to make documentation for llvm.vectorizer.width more clear
> - check unroll and width are powers of two and unroll <= 16 and width <= 64
> - addressed style issues
>
> paul
>
>
>
> On 2013-05-27, at 3:57 PM, Nadav Rotem wrote:
>
> 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<mailto: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><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><mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> <Mail Attachment>
>
>
> <Mail Attachment>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130528/bc691ddb/attachment.html>
More information about the llvm-commits
mailing list