[PATCH] Small refactor on VectorizerHint for deduplication

Arch D. Robison arch.robison at intel.com
Fri Aug 15 12:30:16 PDT 2014


Minor editorial comments.  I'm not an LLVM expert.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1117
@@ +1116,3 @@
+  bool matchesHintMetadataName(MDNode *Node, std::vector<HintType> &HintTypes) {
+    MDString* Name = cast<MDString>(Node->getOperand(0));
+    if (!Name)
----------------
Should the cast be dyn_cast?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1137
@@ +1136,3 @@
+      for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
+        MDNode *Node = cast<MDNode>(LoopID->getOperand(i));
+        // If node in update list, ignore old value.
----------------
Is it safe to assume that the operands are all MDNode and not other kinds of values?  If not, consider not casting here, and instead pass a Value* to "matchesHintMetadataName" and do the check for MDNode-ness there.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1139
@@ +1138,3 @@
+        // If node in update list, ignore old value.
+        if (matchesHintMetadataName(Node, HintTypes))
+          continue;
----------------
The if/continue/else seems a bit wordy.  It would be shorter to write:

if (!matchesHintMetadataNameNode, HintTypes))
    Vals.push_back(Node);

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1182
@@ -1133,2 +1181,3 @@
 
-  MDNode *LoopID;
+  /// The loop this hints belong to.
+  const Loop *TheLoop;
----------------
"this" should be "these".

http://reviews.llvm.org/D4913






More information about the llvm-commits mailing list