[PATCH] D15177: [LoopVectorizer] Refine loop vectorizer's register usage calculator by ignoring specific instructions.

[James Molloy via llvm-commits]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Cong,

Thanks for doing this. I have a bunch of comments below.

Cheers,

James


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1526
@@ -1523,2 +1525,3 @@
   DemandedBits *DB;
+  AssumptionCache *AC;
   const Function *TheFunction;
----------------
Everything else has a (pointless) comment - let's add a (pointless) comment here for consistency's sake.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5174
@@ -5183,1 +5173,3 @@
 
+    // Skip ignored values.
+    if (ValuesToIgnore.count(I))
----------------
Why this change?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5655
@@ +5654,3 @@
+        continue;
+      IncrInst = UI;
+      UserNum++;
----------------
This is not the right way to determine which user (if any) updates the PHI. In fact, it could be a chain of users, and they could be in any order.

The right way is to check the incoming value of the induction PHI from the loop latch.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5667
@@ +5666,3 @@
+      bool IncrInstToIgnore = true;
+      int UserNum = 0;
+      for (User *U : IncrInst->users()) {
----------------
Redefining this variable is confusing - it should be given a unique name.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5680
@@ +5679,3 @@
+    }
+  }
+
----------------
I can't help but feel this is all a bit long-winded. Something like this should work and is a lot shorter (if you don't suffer from STL allergies):

    for (auto &Induction : *Legal->getInductionVars()) {
      auto *PN = KV.first;
      auto *UpdateV = PN->getIncomingValueForBlock(L->getLoopLatch());

      // Check that the PHI is only used by the induction increment (UpdateV) or
      // by GEPs.
      // Then, check that UpdateV is only used by a compare instruction or the loop
      // header PHI.
      if (std::all_of(PN->user_begin(), PN->user_end(), [&](const User *U) {
            return U == UpdateV || isa<GetElementPtrInst>(U);
          }) &&
          std::all_of(UpdateV->user_begin(), UpdateV->user_end(), [&](const User *U) {
            return U == PN || isa<ICmpInst>(U);
          })) {
        ValuesToIgnore.insert(PN);
        ValuesToIgnore.insert(UpdateV);
      }
    }

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5687
@@ +5686,3 @@
+      switch (Inst.getOpcode()) {
+      case Instruction::GetElementPtr:
+        ValuesToIgnore.insert(&Inst);
----------------
Surely this isn't right - GEPs can be vectorized if they have pointer type, right? as in scatter gather?

================
Comment at: test/Transforms/LoopVectorize/X86/vector_max_bandwidth.ll:19
@@ -18,3 +18,3 @@
 ; CHECK-label: foo
-; CHECK: LV: Selecting VF: 16.
+; CHECK: LV: Selecting VF: 32.
 define void @foo() {
----------------
I don't understand why your changes should affect the VF being selected. It should affect the UF only, right?


http://reviews.llvm.org/D15177