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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked an inline comment as done.
ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4091
+            !BS.getScheduleData(VL0)->isPartOfBundle() ||
+            needToScheduleSingleInstruction(VL)) &&
            "tryScheduleBundle should cancelScheduling on failure");
----------------
vporpo wrote:
> 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.
> > 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?
> 

Actually, the check in `extendSchedulingRegion()` is not required, there is an assert instead. Plus, `extendSchedulingRegion()` calls `initScheduleData()`, which then checks if actually need to schedule the given instruction.

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

Ah, sorry, did not check logic correctly. Actually, it is safe to remove this check, was part of the development process, forgot to remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121121