[PATCH] D120364: [SLP] Simplify extendSchedulingRegion and fix a bug in region cap handling

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: ABataev, nikic, fhahn.
Herald added subscribers: bollu, hiraditya, mcrosier.
reames requested review of this revision.
Herald added a project: LLVM.

This change does two things.

First, it uses instruction's comesBefore method to simplify the code significantly.  There's little compile time concern here because getSpillCost already calls comesBefore on every basic block which contains a vectorization candidate.  The only additional times we'll build basic block ordering is when we can't schedule a vector candidate anywhere in the containing block.

Second, it fixes a bug the simpler implementation makes clear.  The previous code was only incrementing the region size if the loop ran - but it exited early if beginning or end of block found.  This radically undercounts region size if initial region starts near one side of the block.  I didn't bother to add a test for this as I hope to remove this limit entirely in the near future.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120364

Files:
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp


Index: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
===================================================================
--- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -7670,26 +7670,15 @@
     LLVM_DEBUG(dbgs() << "SLP:  initialize schedule region to " << *I << "\n");
     return true;
   }
-  // Search up and down at the same time, because we don't know if the new
-  // instruction is above or below the existing scheduling region.
-  BasicBlock::reverse_iterator UpIter =
-      ++ScheduleStart->getIterator().getReverse();
-  BasicBlock::reverse_iterator UpperEnd = BB->rend();
-  BasicBlock::iterator DownIter = ScheduleEnd->getIterator();
-  BasicBlock::iterator LowerEnd = BB->end();
-  while (UpIter != UpperEnd && DownIter != LowerEnd && &*UpIter != I &&
-         &*DownIter != I) {
-    if (++ScheduleRegionSize > ScheduleRegionSizeLimit) {
+
+  if (I->comesBefore(ScheduleStart)) {
+    ScheduleRegionSize += std::distance(I->getIterator(),
+                                            ScheduleStart->getIterator());
+    if (ScheduleRegionSize > ScheduleRegionSizeLimit) {
       LLVM_DEBUG(dbgs() << "SLP:  exceeded schedule region size limit\n");
       return false;
     }
 
-    ++UpIter;
-    ++DownIter;
-  }
-  if (DownIter == LowerEnd || (UpIter != UpperEnd && &*UpIter == I)) {
-    assert(I->getParent() == ScheduleStart->getParent() &&
-           "Instruction is in wrong basic block.");
     initScheduleData(I, ScheduleStart, nullptr, FirstLoadStoreInRegion);
     ScheduleStart = I;
     if (isOneOf(S, I) != I)
@@ -7698,17 +7687,22 @@
                       << "\n");
     return true;
   }
-  assert((UpIter == UpperEnd || (DownIter != LowerEnd && &*DownIter == I)) &&
-         "Expected to reach top of the basic block or instruction down the "
-         "lower end.");
-  assert(I->getParent() == ScheduleEnd->getParent() &&
-         "Instruction is in wrong basic block.");
-  initScheduleData(ScheduleEnd, I->getNextNode(), LastLoadStoreInRegion,
-                   nullptr);
-  ScheduleEnd = I->getNextNode();
+
+  auto *NextI = I->getNextNode();
+  assert(NextI && "tried to vectorize a terminator?");
+  assert(ScheduleEnd->comesBefore(NextI) && "must extend?");
+
+  ScheduleRegionSize += std::distance(ScheduleEnd->getIterator(),
+                                      I->getIterator());
+  if (ScheduleRegionSize > ScheduleRegionSizeLimit) {
+    LLVM_DEBUG(dbgs() << "SLP:  exceeded schedule region size limit\n");
+    return false;
+  }
+
+  initScheduleData(ScheduleEnd, NextI, LastLoadStoreInRegion, nullptr);
+  ScheduleEnd = NextI;
   if (isOneOf(S, I) != I)
     CheckSheduleForI(I);
-  assert(ScheduleEnd && "tried to vectorize a terminator?");
   LLVM_DEBUG(dbgs() << "SLP:  extend schedule region end to " << *I << "\n");
   return true;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120364.410656.patch
Type: text/x-patch
Size: 2861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220222/cbc0a4cb/attachment.bin>