[PATCH] D17191: [LoopVectorize] Annotate versioned loop with noalias metadata

[Michael Zolotukhin via llvm-commits]

mzolotukhin added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:449-455
@@ -447,2 +448,9 @@
 
+  /// \brief Propagate known metadata from one instruction to another.
+  void propagateMetadata(Instruction *To, const Instruction *From);
+
+  /// \brief Propagate known metadata from one instruction to a vector of
+  /// others.
+  void propagateMetadata(SmallVectorImpl<Value *> &To, const Instruction *From);
+
   /// This is a helper class that holds the vectorizer state. It maps scalar
----------------
anemet wrote:
> mzolotukhin wrote:
> > These functions are generally useful, not only in LoopVectorizer - e.g. we have their duplicates in SLPVectorizer. While you're at it, could you please move them out to a common place (and commit as a separate change)?
> They are not really duplicates.  The one in LV copies from instruction to one/many.  The one in SLP copies many to one and merges them in the process in a metadata-specific way.
> 
> You can obviously still refactor the common parts or create a superset but I think that will probably be harder to read at the end.
> 
> What do you think?
That's true, but to me it looks like SLP version is just more general, where we need to merge several, potentially different sets of attributes. In LV case we can also think of such merging, but all the sets are the same, so the merge is trivial.

The reason I think it might be useful is that it's currently easy to forget to update both versions. E.g. when I added propagation of 'nontemporal' hints, I had to fix that in two very similar places.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:659-663
@@ -642,2 +658,7 @@
   }
+
+  // If the loop was versioned with memchecks, add the corresponding no-alias
+  // metadata.
+  if (LVer && (isa<LoadInst>(From) || isa<StoreInst>(From)))
+    LVer->annotateInstWithNoAlias(To, From);
 }
----------------
anemet wrote:
> mzolotukhin wrote:
> > Does it belong here? I.e. should it really be a part of `propagateMetadata`? Probably that's fine with the current usage of `propagateMetadata` but it might become surprising if one decides to use this function somewhere else.
> Well, that depends on what you respond to the above but semantically, I do think it belongs here.
> 
> Here we're propagating the metadata from the "versioned" scalar loop (which is never created) to the vector loop.  Let me know if this is not clear and I should add a comment.
Yeah, if you feel convinced that there is no reason in factoring `propagateMetadata` out, then it's fine to keep this code here I guess.

================
Comment at: test/Transforms/LoopVectorize/noalias-md.ll:1-2
@@ +1,3 @@
+; RUN: opt -basicaa -loop-vectorize -force-vector-width=2 \
+; RUN:     -S < %s | FileCheck %s -check-prefix=BOTH -check-prefix=LV
+; RUN: opt -basicaa -scoped-noalias -loop-vectorize -dse -force-vector-width=2 \
----------------
Don't you need `-scoped-alias` in the first command as well?


http://reviews.llvm.org/D17191