Skip to content

Commit 5628462

Browse files
author
Vasileios Porpodas
committed
[SandboxVec][DAG] Cleanup: Move callback registration from Scheduler to DAG
This is a refactoring patch that moves the callback registration for getting notified about new instructions from the scheduler to the DAG. This makes sense from a design and testing point of view: - the DAG should not rely on the scheduler for getting notified - the notifiers don't need to be public - it's easier to test the notifiers directly from within the DAG unit tests
1 parent 1be9827 commit 5628462

File tree

3 files changed

+54
-12
lines changed

3 files changed

+54
-12
lines changed

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,9 @@ class DependencyGraph {
290290
/// The DAG spans across all instructions in this interval.
291291
Interval<Instruction> DAGInterval;
292292

293+
Context *Ctx = nullptr;
294+
std::optional<Context::CallbackID> CreateInstrCB;
295+
293296
std::unique_ptr<BatchAAResults> BatchAA;
294297

295298
enum class DependencyType {
@@ -325,9 +328,26 @@ class DependencyGraph {
325328
/// chain.
326329
void createNewNodes(const Interval<Instruction> &NewInterval);
327330

331+
/// Called by the callbacks when a new instruction \p I has been created.
332+
void notifyCreateInstr(Instruction *I) {
333+
getOrCreateNode(I);
334+
// TODO: Update the dependencies for the new node.
335+
// TODO: Update the MemDGNode chain to include the new node if needed.
336+
}
337+
328338
public:
329339
DependencyGraph(AAResults &AA)
330340
: BatchAA(std::make_unique<BatchAAResults>(AA)) {}
341+
/// This constructor also registers callbacks.
342+
DependencyGraph(AAResults &AA, Context &Ctx)
343+
: Ctx(&Ctx), BatchAA(std::make_unique<BatchAAResults>(AA)) {
344+
CreateInstrCB = Ctx.registerCreateInstrCallback(
345+
[this](Instruction *I) { notifyCreateInstr(I); });
346+
}
347+
~DependencyGraph() {
348+
if (CreateInstrCB)
349+
Ctx->unregisterCreateInstrCallback(*CreateInstrCB);
350+
}
331351

332352
DGNode *getNode(Instruction *I) const {
333353
auto It = InstrToNodeMap.find(I);
@@ -354,11 +374,6 @@ class DependencyGraph {
354374
Interval<Instruction> extend(ArrayRef<Instruction *> Instrs);
355375
/// \Returns the range of instructions included in the DAG.
356376
Interval<Instruction> getInterval() const { return DAGInterval; }
357-
/// Called by the scheduler when a new instruction \p I has been created.
358-
void notifyCreateInstr(Instruction *I) {
359-
getOrCreateNode(I);
360-
// TODO: Update the dependencies for the new node.
361-
}
362377
void clear() {
363378
InstrToNodeMap.clear();
364379
DAGInterval = {};

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Scheduler.h

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,6 @@ class Scheduler {
106106
std::optional<BasicBlock::iterator> ScheduleTopItOpt;
107107
// TODO: This is wasting memory in exchange for fast removal using a raw ptr.
108108
DenseMap<SchedBundle *, std::unique_ptr<SchedBundle>> Bndls;
109-
Context &Ctx;
110-
Context::CallbackID CreateInstrCB;
111109

112110
/// \Returns a scheduling bundle containing \p Instrs.
113111
SchedBundle *createBundle(ArrayRef<Instruction *> Instrs);
@@ -137,11 +135,8 @@ class Scheduler {
137135
Scheduler &operator=(const Scheduler &) = delete;
138136

139137
public:
140-
Scheduler(AAResults &AA, Context &Ctx) : DAG(AA), Ctx(Ctx) {
141-
CreateInstrCB = Ctx.registerCreateInstrCallback(
142-
[this](Instruction *I) { DAG.notifyCreateInstr(I); });
143-
}
144-
~Scheduler() { Ctx.unregisterCreateInstrCallback(CreateInstrCB); }
138+
Scheduler(AAResults &AA, Context &Ctx) : DAG(AA, Ctx) {}
139+
~Scheduler() {}
145140

146141
bool trySchedule(ArrayRef<Instruction *> Instrs);
147142

llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -798,3 +798,35 @@ define void @foo(ptr %ptr, i8 %v1, i8 %v2, i8 %v3, i8 %v4, i8 %v5) {
798798
EXPECT_EQ(S1N->getNumUnscheduledSuccs(), 0u); // S1 is scheduled
799799
}
800800
}
801+
802+
TEST_F(DependencyGraphTest, CreateInstrCallback) {
803+
parseIR(C, R"IR(
804+
define void @foo(ptr %ptr, i8 %v1, i8 %v2, i8 %v3, i8 %arg) {
805+
store i8 %v1, ptr %ptr
806+
store i8 %v2, ptr %ptr
807+
store i8 %v3, ptr %ptr
808+
ret void
809+
}
810+
)IR");
811+
llvm::Function *LLVMF = &*M->getFunction("foo");
812+
sandboxir::Context Ctx(C);
813+
auto *F = Ctx.createFunction(LLVMF);
814+
auto *BB = &*F->begin();
815+
auto It = BB->begin();
816+
auto *S1 = cast<sandboxir::StoreInst>(&*It++);
817+
[[maybe_unused]] auto *S2 = cast<sandboxir::StoreInst>(&*It++);
818+
auto *S3 = cast<sandboxir::StoreInst>(&*It++);
819+
820+
// Check new instruction callback.
821+
sandboxir::DependencyGraph DAG(getAA(*LLVMF), Ctx);
822+
DAG.extend({S1, S3});
823+
auto *Arg = F->getArg(3);
824+
auto *Ptr = S1->getPointerOperand();
825+
sandboxir::StoreInst *NewS =
826+
sandboxir::StoreInst::create(Arg, Ptr, Align(8), S3->getIterator(),
827+
/*IsVolatile=*/true, Ctx);
828+
auto *NewSN = DAG.getNode(NewS);
829+
EXPECT_TRUE(NewSN != nullptr);
830+
// TODO: Check the dependencies to/from NewSN after they land.
831+
// TODO: Check the MemDGNode chain.
832+
}

0 commit comments

Comments
 (0)