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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2720-2726
+  /// It does not schedules instructions, which are not memory read/write
+  /// instructions and their operands are either constants, or arguments, or
+  /// phis, or instructions from others blocks, or their users are phis or from
+  /// the other blocks. The resulting vector instructions can be placed at the
+  /// beginning of the basic block without scheduling (if operands does not need
+  /// to be scheduled) or at the end of the block (if users are outside of the
+  /// block). It allows to save some compile time and memory used by the
----------------
Some of this text repeats multiple times in the patch, it is probably better to avoid too much repetition.

What would also be nice to have here is some brief explanation about the design, like when exactly a ScheduleData entry is assigned to an Instruction.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7953
   for (Instruction *I = FromI; I != ToI; I = I->getNextNode()) {
-    ScheduleData *SD = ScheduleDataMap[I];
+    ScheduleData *SD = ScheduleDataMap.lookup(I);
+    // No need to allocate data for non-schedulable instructions.
----------------
move this under `if (doesNotNeedToSchedule(I))` ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4091
+            !BS.getScheduleData(VL0)->isPartOfBundle() ||
+            needToScheduleSingleInstruction(VL)) &&
            "tryScheduleBundle should cancelScheduling on failure");
----------------
ABataev wrote:
> vporpo wrote:
> > ABataev wrote:
> > > 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.
> > I am probably missing something about the design here.
> > 
> > If the check is false for `!BS.getScheduleData(VL0) || !BS.getScheduleData(VL0)->isPartOfBundle()` then it means that `VL0` is part of a real bundle with more than one instruction in the bundle. In other words, how can we end up in such a situation where `VL0` is in a multi-instruction bundle and still have a single instruction in `VL` that needs scheduling?
> > 
> > Let me explain. If we had only a single instruction that needs scheduling, then this could be either:
> > (i) `VL0`, in which case it should be a single-instruction bundle so `isPartOfBundle()` would be false, or 
> > (ii) some other instruction in `VL`, in which case `VL0` should not require scheduling so `getScheduleData(VL0)` should be false because we no longer assign `ScheduleData` objects to instructions that don't need scheduling.
> We allocate ScheduleData not only for schedulable instruction, but also for all instruction in between, but since they don't have next elements, they are not considered to be part of bundle. Function `isPartOfBundle()` is not enough here, because if we have only single schedulable instruction, `isPartOfBundle()` will still return false here, because this instruction doers not have next element in bundle and TE is not set yet.
> Here we have a corner case, where only single instruction from the VL must be scheduled and `isPartOfBundle()` still returns false.
> We allocate ScheduleData not only for schedulable instruction, but also for all instruction in between,
I see, `initScheduleData()` won't allocate ScheduleData unless it is schedulable, but `extendSchedulingRegion()` will do so without checking. Why is this done this way, and not for example always skip ScheduleData?

> Here we have a corner case, where only single instruction from the VL must be scheduled and isPartOfBundle() still returns false.
I don't follow. If `isPartOfBundle()` returns false then `!BS.getScheduleData(VL0) || !BS.getScheduleData(VL0)->isPartOfBundle()` is true, and we don't need `needToScheduleSingelInstruction(VL)`. The assertion would pass anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121121