[PATCH] Remove unroll pragma metadata after it is used

Mark Heffernan meheff at google.com
Thu Jul 17 15:11:59 PDT 2014


Thanks for the comments.  I'll immediately follow this with the updated patch.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:286
@@ +285,3 @@
+  // First remove any existing loop unrolling metadata.
+  SmallVector<Value *, 4> Vals(1);
+  for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
----------------
hfinkel at anl.gov wrote:
> I suppose this 1 Value* is a placeholder for the self-reference loop-id. Should comment. Also, you expect this to be nullptr, right? You should explicit specify that so it is clear.
Added a comment, and slightly changed the code so it is less surprising.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:301
@@ +300,3 @@
+      MDNode::get(Context, {MDString::get(Context, "llvm.loop.unroll.enable"),
+                            ConstantInt::get(Type::getInt1Ty(Context), 0)});
+  Vals.push_back(DisableNode);
----------------
hfinkel at anl.gov wrote:
> This does not seem to match the language reference, which says "Note that setting llvm.loop.vectorize.unroll to 1 disables unrolling of the loop." -- Is that out of date?
"llvm.loop.vectorize.unroll" is for the vectorizer unroller (interleaver), this is the concatenation loop unroller which uses "llvm.loop.unroll.*".  For the concatenation unroller, setting "llvm.loop.unroll.enable" to 0 disables unrolling. 

And now I see that "llvm.loop.unroll.*" metadata needs an entry in LangRef.rst and the bit about "llvm.loop.vectorize.unroll" should be changed to match the terminology in LanguageExtensions.rst (use "interleave" instead of "unroll" to avoid confusing it with the loop unroller).  I'll fix this in a follow up change.

http://reviews.llvm.org/D4571






More information about the llvm-commits mailing list