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

[James Molloy]

Hi Sam,

I've given an initial review, but I'm not sure this will catch all the interesting cases.

You don't recurse anywhere, so you only check single operations. For example, you'll miss this:

  %1 = zext i32 %a to i64
  %2 = add %1, %1
  %3 = add %2, %1
  %4 = trunc i64 %3 to i32

There are more complex cases it won't catch either, but I'm less worried about that as primarily this comes up with C promotion rules, so everything gets promoted to the same size (i32) which simplifies things somewhat.

Cheers,

James


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:891
@@ -889,1 +890,3 @@
 
+  // Return the vectorized type or a clamped type that may have been
+  // previously identified.
----------------
This needs doxygen comment style (///) and also a description of what a clamped type is.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:895
@@ +894,3 @@
+
+  bool isFreeCast(Instruction *I, unsigned VF);
+
----------------
This needs documentation. Does it assume I is a CastInst?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4379
@@ -4365,4 +4378,3 @@
 unsigned LoopVectorizationCostModel::expectedCost(unsigned VF) {
   unsigned Cost = 0;
   // For each block.
----------------
Whitespace change.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4387
@@ -4374,2 +4386,3 @@
     // For each instruction in the old loop.
-    for (BasicBlock::iterator it = BB->begin(), e = BB->end(); it != e; ++it) {
+    for (BasicBlock::reverse_iterator it = BB->rbegin(), e = BB->rend();
+         it != e; ++it) {
----------------
Please add a comment as to why we're reversing through the loop.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4484
@@ +4483,3 @@
+bool
+LoopVectorizationCostModel::isFreeCast(Instruction *I, unsigned VF) {
+  CastInst *CI = cast<CastInst>(I);
----------------
There's no need for this to be split over two lines - the "bool" will fit inside the 80 char limit.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4490
@@ +4489,3 @@
+  // The search starts from truncs towards the possible extend instructions,
+  // so these would of been found eariler.
+  if (I->getOpcode() != Instruction::Trunc) {
----------------
"so any extends would have been found earlier." - SPAG and clarification

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4491
@@ +4490,3 @@
+  // so these would of been found eariler.
+  if (I->getOpcode() != Instruction::Trunc) {
+    if (FreeCasts.count(I))
----------------
The canonical way to do this is "if (!isa<TruncInst>(I))"

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4492
@@ +4491,3 @@
+  if (I->getOpcode() != Instruction::Trunc) {
+    if (FreeCasts.count(I))
+      return true;
----------------
This can simply be "return FreeCasts.count(I);"

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4498
@@ +4497,3 @@
+
+  // Should only be used to check integer operations.
+  if (!DstTy->isIntegerTy())
----------------
I know it's pedantic, but this comment implies that it was the user's fault that we were called on a non-integer operation. Actually, we just don't deal with them, so I'd suggest rewriting to something like:

"// We only care about integer operations."

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4514
@@ +4513,3 @@
+    // constants within the size limit.
+    for (unsigned i = 0; i < ChainInst->getNumOperands(); ++i) {
+
----------------
Canonical form has local variables as UpperCamelCase.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4516
@@ +4515,3 @@
+
+      if (isa<ConstantInt>(ChainInst->getOperand(i))) {
+        auto *ConstInt = cast<ConstantInt>(ChainInst->getOperand(i));
----------------
You can merge this and the next line with:

    if (ConstantInst *CI = dyn_cast<ConstantInst>(ChainInst->getOperand(i)))

http://reviews.llvm.org/D9822

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