[PATCH] D117951: [SLP] Optimize reschedule of previously scheduled bundle member [NFC]

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2599
+    void unschedule(ScheduleData *SD) {
+      assert(SD && SD->isSchedulingEntity() && SD->IsScheduled);
+      LLVM_DEBUG(dbgs() << "SLP:   unschedule " << *SD << "\n");
----------------
ABataev wrote:
> Asserting message?
Doesn't seem to add any value and we are inconsistent about them across codebase.

I really don't care here, if you want it added, please give me wording and I'll copy-and-paste.  


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2602-2603
+
+      SmallVector<ScheduleData*> Worklist;
+      Worklist.push_back(SD);
+
----------------
ABataev wrote:
> `SmallVector<ScheduleData *> Worklist(1, SD);`
Not idiomatic, and frankly, confusing.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2613
+        // If BundleMember is a vector bundle, its operands may have been
+        // reordered duiring buildTree(). We therefore need to get its operands
+        // through the TreeEntry.
----------------
ABataev wrote:
> `during`
Comment copied from existing code.  I will fix both in a followon commit.  


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2627
+          assert(In &&
+                 (isa<ExtractValueInst>(In) || isa<ExtractElementInst>(In) ||
+                  In->getNumOperands() == TE->getNumOperands()) &&
----------------
ABataev wrote:
> Can we get PHINode here? Plus, extracts and PHIs are completely excluded from scheduling, I assume you can skip such instructions. Same about insertelements
This is a good question to which I didn't know the answer.  

>From digging, it looks like we can't directly reach this code with a phi.  Reason being that scheduling a phi bundle will always fail without trying to extend.  

On the other hand, the phi could be an operand to another bundle which is scheduled, and then unscheduled.  But, and I think this is the key bit, the phis should never be in the resulting scheduling window.  As such, they can't ever be marked "scheduled" and we'll push them to the worklist, then ignore them.

As such, the code appears to be correct as written, if admittedly, accidentally.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117951/new/

https://reviews.llvm.org/D117951