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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D121121#3374247 <https://reviews.llvm.org/D121121#3374247>, @vporpo wrote:

> 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.

Where do you want to see this info?

> 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?

I would like to save some memory too, not only compile time. Why do we need to allocate ScheduleData for the instructions that should not be scheduled?



================
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);
----------------
vporpo wrote:
> We may need to limit the number of users to a small integer because in some pathological cases we may have thousands of them.
Good point, will do this.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2356
         BundleMember->TE = Last;
-        BundleMember->Lane = Lane;
-        ++Lane;
----------------
vporpo wrote:
> Why are we removing `Lane`?
It is no needed anymore (actually used only in one place). Plus, it gets invalidated after graph reordering and it should be recalculated. Instead, better to find the corresponding instruction directly rather than keep this Lane and then recalculate it after graph reordering. Plus, saves some memory.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2397
+           ++Lane) {
+        if (areAllOperandsNonInsts(VL[Lane]) && isUsedOutsideBlock(VL[Lane]))
+          continue;
----------------
vporpo wrote:
> 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` ?
Rather doubt we need a cache. The checks are pretty simple and should not take much time to perform.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2580
     bool isPartOfBundle() const {
-      return NextInBundle != nullptr || FirstInBundle != this;
+      return NextInBundle != nullptr || FirstInBundle != this || TE;
     }
----------------
vporpo wrote:
> Do we need the checks `NextInBundle != nullptr || FirstInBundle != this` ? Shouldn't the check `TE != nullptr` be sufficient?
Yes, still need these checks since this member function is used before we actually assigning TE.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4091
+            !BS.getScheduleData(VL0)->isPartOfBundle() ||
+            needToScheduleSingleInstruction(VL)) &&
            "tryScheduleBundle should cancelScheduling on failure");
----------------
vporpo wrote:
> 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. 
> 
isPartOfBundle is not powerfull enough to check if we have just a single instruction that requires scheduling. We don't have assigned TE yet and need an extra check here for a single schedulable instruction.


================
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;
----------------
vporpo wrote:
> Perhaps move these checks in a method`TreeEntry::doesNotNeedScheduling()` ?
doesNotNeedScheduling works with a list of values, not with TreeEntry. And in some cases we need to call it for just a list. This check is needed only here, other cases do not require anything like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121121