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

[Cong Hou via llvm-commits]

congh added a comment.

Thank you very much for the review, James! Please check my inline replies.


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

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5174
@@ -5183,1 +5173,3 @@
 
+    // Skip ignored values.
+    if (ValuesToIgnore.count(I))
----------------
jmolloy wrote:
> Why this change?
This is actually fixing a previous bug: we need to update OpenIntervals by removing instructions that end at the current location, even when we ignore values in ValuesToIgnore. Those instructions to remove won't be considered later.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5680
@@ +5679,3 @@
+    }
+  }
+
----------------
jmolloy wrote:
> 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);
>       }
>     }
Thank you very much for the suggested code. I love STL. I have applied your code in this patch.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5687
@@ +5686,3 @@
+      switch (Inst.getOpcode()) {
+      case Instruction::GetElementPtr:
+        ValuesToIgnore.insert(&Inst);
----------------
jmolloy wrote:
> Surely this isn't right - GEPs can be vectorized if they have pointer type, right? as in scatter gather?
I have added the check and now we only ignore this instruction if its last operand is an induction var. Do you have better implementation suggestion here?

BTW, does LLVM support scatter/gather now? It seems that vectorizeBlockInLoop doesn't vectorize this operation. 

================
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() {
----------------
jmolloy wrote:
> I don't understand why your changes should affect the VF being selected. It should affect the UF only, right?
With the patch http://reviews.llvm.org/D8943, VF is also determined by register usage when -vectorizer-maximize-bandwidth is turned on (as is in this test file).


http://reviews.llvm.org/D15177