[PATCH] D121121: [SLP]Do not schedule instructions with constants/argument/phi operands and external users.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

I think this needs some text in the code to describe the high-level design changes in the scheduler introduced by this patch. 
There are a couple of things to explain like:

- When an instruction is not scheduled (which is also explained in the comments of `isUsedOutsideBlock()` and `areAllOperandsNonInsts()`).
- Changes in the scheduler's design and data structures: If I understand correctly the instructions that are skipped don't get a `ScheduleData` object assigned to them.

Regarding the last point, couldn't we assign a `ScheduleData` object even to the instructions that are not scheduled and still save compilation time?  I would assume that most of the compile-time overhead comes from `calculateDependencies()`, so as long as we don't calculate dependencies beyond the instructions that are marked to be skipped, then we should still save compilation time. I think this would make this change a bit less intrusive as it would not change the design of the scheduler much. What do you think?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:799
+    return true;
+  return !I->mayReadOrWriteMemory() && all_of(I->users(), [I](User *U) {
+    auto *IU = dyn_cast<Instruction>(U);
----------------
We may need to limit the number of users to a small integer because in some pathological cases we may have thousands of them.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2356
         BundleMember->TE = Last;
-        BundleMember->Lane = Lane;
-        ++Lane;
----------------
Why are we removing `Lane`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2394-2396
+      for (ScheduleData *BundleMember = Bundle.getValue();
+           BundleMember || (!BundleMember && Lane > 0 && Lane != VL.size());
+           ++Lane) {
----------------
This is a bit hard to read. I think it could be simplified by iterating across lanes in the for loop `for (unsigned Lane = 0, Lanes = VL.size(); Lane != Lanes; ++Lane)` and then setting `BundleMember` inside the for loop.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2397
+           ++Lane) {
+        if (areAllOperandsNonInsts(VL[Lane]) && isUsedOutsideBlock(VL[Lane]))
+          continue;
----------------
1. I think we need a function like `mustBeScheduled(Value *V)` that calls `areAllOperandsNonInsts()` and `isUsedOutsideBlock()`.

2. Also these checks show up in more than one place. Do you think we could check once and and cache the outcome of the check in a map? Perhaps add a `NeedsScheduling` field in `ScheduleData` ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2580
     bool isPartOfBundle() const {
-      return NextInBundle != nullptr || FirstInBundle != this;
+      return NextInBundle != nullptr || FirstInBundle != this || TE;
     }
----------------
Do we need the checks `NextInBundle != nullptr || FirstInBundle != this` ? Shouldn't the check `TE != nullptr` be sufficient?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3887
+static bool needToScheduleSingleInstruction(ArrayRef<Value *> VL) {
+  Value *Scheduled = nullptr;
+  for (Value *V : VL) {
----------------
perhaps rename it to `NeedsScheduling`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4091
+            !BS.getScheduleData(VL0)->isPartOfBundle() ||
+            needToScheduleSingleInstruction(VL)) &&
            "tryScheduleBundle should cancelScheduling on failure");
----------------
I don't understand why `needToScheduleSingleInstruction(VL)` needs to be here. If I understand correctly this assertion checks that if we have failed to schedule VL, then cacnelScheduling has worked correctly and `VL0` (and probably the rest of the values?) are not marked as part of a bundle. So `needToScheduleSingleInstruction(VL)` executes if `VL0` is part of a bundle. 



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6445-6446
+  // should not be scheduled.
+  if (E->State != TreeEntry::NeedToGather &&
+      doesNotNeedToSchedule(E->Scalars)) {
+    BasicBlock::iterator InsertPt;
----------------
Perhaps move these checks in a method`TreeEntry::doesNotNeedScheduling()` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121121