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

> Where do you want to see this info?

Perhaps right above `struct BlockScheduling` ?

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

Makes sense. Please mention this in the design description text too.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:796
+static bool isUsedOutsideBlock(Value *V) {
+  constexpr int UsesLimit = 8;
+  auto *I = dyn_cast<Instruction>(V);
----------------
Nit: Please add a one line comment saying that this limit is to save compilation time when instrs have too many uses.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121121