[PATCH] Reducing the costs of cast instructions to enable more vectorization of smaller types in LoopVectorize

[Michael Zolotukhin]

Hi Sam,

Some initial comments from me inline:


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:952-953
@@ -924,1 +951,4 @@
+  std::map<unsigned, std::map<Instruction*, Type*>> ClampedInstrTys;
+  // Sext and Zext instructions that will disappear due to vectorization.
+  std::map<unsigned, SmallPtrSet<Instruction*, 4>> FreeCasts;
 };
----------------
Please add a comment that the key in this map is VectorizationFactor.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2601-2602
@@ +2600,4 @@
+        // Replace the operand with the new value if it is different.
+        if (Clamped != I)
+          *II = (Value*)Clamped;
+      }
----------------
Should it be `if(Clamped != *II)` ? Since `Clamped` originally is just an operand of `I`, this condition is almost always true, or am I missing something?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2612
@@ +2611,3 @@
+  if (I->isBinaryOp()) {
+    IRBuilder<> builder(I);
+    Value *Op0 = I->getOperand(0);
----------------
Is it used anywhere?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4149
@@ -4047,2 +4148,3 @@
 unsigned LoopVectorizationCostModel::getWidestType() {
+
   unsigned MaxWidth = 8;
----------------
Please remove this unneeded change.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4478-4480
@@ -4373,3 +4477,5 @@
 
-    // For each instruction in the old loop.
-    for (BasicBlock::iterator it = BB->begin(), e = BB->end(); it != e; ++it) {
+    // For each instruction in the old loop. This has been reversed so that
+    // the search can begin from trunc insts so that we can find free casts
+    // and smaller types for operations.
+    for (BasicBlock::reverse_iterator it = BB->rbegin(), e = BB->rend();
----------------
Please reword the comment so that it doesn't refer to the previous version: e.g. instead of "This has been reversed.." use something like "iterate in reverse order because..".
In the current form the comment makes sense in the context of the patch, but it would look a bit strange when you don't see the context.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4481
@@ +4480,3 @@
+    // and smaller types for operations.
+    for (BasicBlock::reverse_iterator it = BB->rbegin(), e = BB->rend();
+         it != e; ++it) {
----------------
Maybe use `auto` here?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4484
@@ -4376,3 +4483,3 @@
       // Skip dbg intrinsics.
-      if (isa<DbgInfoIntrinsic>(it))
+      if (isa<DbgInfoIntrinsic>(*it))
         continue;
----------------
This change looks strange. Is it correct at all?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4850-4851
@@ +4849,4 @@
+      }
+    }
+    else if (isFreeCast(I, VF))
+      return 0;
----------------
`else if` should be on the same line as `}`.
Actually, you could just run clang-format on the patch to avoid such issues.

http://reviews.llvm.org/D9822

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/