[PATCH] D121121: [SLP]Do not schedule instructions with constants/argument/phi operands and external users.
Vasileios Porpodas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 10 17:52:30 PST 2022
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
More information about the llvm-commits
mailing list