-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[SandboxIR] Add callbacks for instruction insert/remove/move ops #112965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
56ffef4
f361d5c
e84e9e6
fa875d7
9068541
18ed35d
35a2074
367e5b5
3ef32de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -35,17 +35,20 @@ Value *Context::registerValue(std::unique_ptr<Value> &&VPtr) { | |||||
| assert(VPtr->getSubclassID() != Value::ClassID::User && | ||||||
| "Can't register a user!"); | ||||||
|
|
||||||
| Value *V = VPtr.get(); | ||||||
| [[maybe_unused]] auto Pair = | ||||||
| LLVMValueToValueMap.insert({VPtr->Val, std::move(VPtr)}); | ||||||
| assert(Pair.second && "Already exists!"); | ||||||
|
|
||||||
| // Track creation of instructions. | ||||||
| // Please note that we don't allow the creation of detached instructions, | ||||||
| // meaning that the instructions need to be inserted into a block upon | ||||||
| // creation. This is why the tracker class combines creation and insertion. | ||||||
| if (auto *I = dyn_cast<Instruction>(VPtr.get())) | ||||||
| if (auto *I = dyn_cast<Instruction>(V)) { | ||||||
| getTracker().emplaceIfTracking<CreateAndInsertInst>(I); | ||||||
| runInsertInstrCallbacks(I); | ||||||
| } | ||||||
|
|
||||||
| Value *V = VPtr.get(); | ||||||
| [[maybe_unused]] auto Pair = | ||||||
| LLVMValueToValueMap.insert({VPtr->Val, std::move(VPtr)}); | ||||||
| assert(Pair.second && "Already exists!"); | ||||||
| return V; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -660,4 +663,54 @@ Module *Context::createModule(llvm::Module *LLVMM) { | |||||
| return M; | ||||||
| } | ||||||
|
|
||||||
| void Context::runRemoveInstrCallbacks(Instruction *I) { | ||||||
| for (const auto &CBEntry : RemoveInstrCallbacks) | ||||||
| CBEntry.second(I); | ||||||
| } | ||||||
|
|
||||||
| void Context::runInsertInstrCallbacks(Instruction *I) { | ||||||
| for (auto &CBEntry : InsertInstrCallbacks) | ||||||
| CBEntry.second(I); | ||||||
| } | ||||||
|
|
||||||
| void Context::runMoveInstrCallbacks(Instruction *I, const BBIterator &WhereIt) { | ||||||
| for (auto &CBEntry : MoveInstrCallbacks) | ||||||
| CBEntry.second(I, WhereIt); | ||||||
| } | ||||||
|
|
||||||
| Context::CallbackID Context::NextCallbackID = 0; | ||||||
|
|
||||||
| int Context::registerRemoveInstrCallback(RemoveInstrCallback CB) { | ||||||
| CallbackID ID = NextCallbackID++; | ||||||
| RemoveInstrCallbacks[ID] = CB; | ||||||
| return ID; | ||||||
| } | ||||||
| void Context::unregisterRemoveInstrCallback(CallbackID ID) { | ||||||
| [[maybe_unused]] bool erased = RemoveInstrCallbacks.erase(ID); | ||||||
| assert(erased && | ||||||
| "Callback ID not found in RemoveInstrCallbacks during deregistration"); | ||||||
| } | ||||||
|
|
||||||
| int Context::registerInsertInstrCallback(InsertInstrCallback CB) { | ||||||
| CallbackID ID = NextCallbackID++; | ||||||
| InsertInstrCallbacks[ID] = CB; | ||||||
| return ID; | ||||||
| } | ||||||
| void Context::unregisterInsertInstrCallback(CallbackID ID) { | ||||||
| [[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID); | ||||||
|
||||||
| [[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID); | |
| [[maybe_unused]] bool Erased = InsertInstrCallbacks.erase(ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| #include "llvm/SandboxIR/Value.h" | ||
| #include "llvm/Support/SourceMgr.h" | ||
| #include "gmock/gmock-matchers.h" | ||
| #include "gmock/gmock-more-matchers.h" | ||
| #include "gtest/gtest.h" | ||
|
|
||
| using namespace llvm; | ||
|
|
@@ -5962,3 +5963,87 @@ TEST_F(SandboxIRTest, CheckClassof) { | |
| EXPECT_NE(&sandboxir::CLASS::classof, &sandboxir::Instruction::classof); | ||
| #include "llvm/SandboxIR/Values.def" | ||
| } | ||
|
|
||
| TEST_F(SandboxIRTest, InstructionCallbacks) { | ||
| parseIR(C, R"IR( | ||
| define void @foo(ptr %ptr, i8 %val) { | ||
| ret void | ||
| } | ||
| )IR"); | ||
| Function &LLVMF = *M->getFunction("foo"); | ||
| sandboxir::Context Ctx(C); | ||
|
|
||
| auto &F = *Ctx.createFunction(&LLVMF); | ||
| auto &BB = *F.begin(); | ||
| sandboxir::Argument *Ptr = F.getArg(0); | ||
| sandboxir::Argument *Val = F.getArg(1); | ||
| sandboxir::Instruction *Ret = &BB.front(); | ||
|
|
||
| SmallVector<sandboxir::Instruction *> Inserted; | ||
| auto InsertCbId = Ctx.registerInsertInstrCallback( | ||
| [&Inserted](sandboxir::Instruction *I) { Inserted.push_back(I); }); | ||
|
|
||
| SmallVector<sandboxir::Instruction *> Removed; | ||
| auto RemoveCbId = Ctx.registerRemoveInstrCallback( | ||
| [&Removed](sandboxir::Instruction *I) { Removed.push_back(I); }); | ||
|
|
||
| // Keep the moved instruction and the instruction pointed by the Where | ||
| // iterator so we can check both callback arguments work as expected. | ||
| SmallVector<std::pair<sandboxir::Instruction *, sandboxir::Instruction *>> | ||
| Moved; | ||
| auto MoveCbId = Ctx.registerMoveInstrCallback( | ||
| [&Moved](sandboxir::Instruction *I, const sandboxir::BBIterator &Where) { | ||
| // Use a nullptr to signal "move to end" to keep it single. We only | ||
| // have a basic block in this test case anyway. | ||
| if (Where == Where.getNodeParent()->end()) | ||
| Moved.push_back(std::make_pair(I, nullptr)); | ||
| else | ||
| Moved.push_back(std::make_pair(I, &*Where)); | ||
| }); | ||
|
|
||
| Ctx.save(); | ||
| auto *NewI = sandboxir::StoreInst::create(Val, Ptr, /*Align=*/std::nullopt, | ||
| Ret->getIterator(), Ctx); | ||
| EXPECT_THAT(Inserted, testing::ElementsAre(NewI)); | ||
| EXPECT_THAT(Removed, testing::IsEmpty()); | ||
| EXPECT_THAT(Moved, testing::IsEmpty()); | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A drive-by comment: How about testing insertBefore and insertAfter?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed this offline because @vporpo's prototype only called them on create/erase, but called them insert/remove. We only need create/erase/move callbacks for the vectorizer, so we're only adding those for now, and I have renamed them to create/erase callbacks. This leaves space to add proper insert/remove callbacks in the future if we (or some future user of Sandbox IR) needs them. Thus, no need to test those here as they have no associated callbacks for now. |
||
| Ret->moveBefore(NewI); | ||
| EXPECT_THAT(Inserted, testing::ElementsAre(NewI)); | ||
| EXPECT_THAT(Removed, testing::IsEmpty()); | ||
| EXPECT_THAT(Moved, testing::ElementsAre(std::make_pair(Ret, NewI))); | ||
|
|
||
| Ret->eraseFromParent(); | ||
| EXPECT_THAT(Inserted, testing::ElementsAre(NewI)); | ||
| EXPECT_THAT(Removed, testing::ElementsAre(Ret)); | ||
| EXPECT_THAT(Moved, testing::ElementsAre(std::make_pair(Ret, NewI))); | ||
|
|
||
| NewI->eraseFromParent(); | ||
| EXPECT_THAT(Inserted, testing::ElementsAre(NewI)); | ||
| EXPECT_THAT(Removed, testing::ElementsAre(Ret, NewI)); | ||
| EXPECT_THAT(Moved, testing::ElementsAre(std::make_pair(Ret, NewI))); | ||
|
|
||
| // Check that after revert the callbacks have been called for the inverse | ||
| // operations of the changes made so far. | ||
| Ctx.revert(); | ||
| EXPECT_THAT(Inserted, testing::ElementsAre(NewI, NewI, Ret)); | ||
| EXPECT_THAT(Removed, testing::ElementsAre(Ret, NewI, NewI)); | ||
| EXPECT_THAT(Moved, testing::ElementsAre(std::make_pair(Ret, NewI), | ||
| std::make_pair(Ret, nullptr))); | ||
|
|
||
| // Check that deregistration works. Do an operation of each type after | ||
| // deregistering callbacks and check. | ||
| Inserted.clear(); | ||
| Removed.clear(); | ||
| Moved.clear(); | ||
| Ctx.unregisterInsertInstrCallback(InsertCbId); | ||
| Ctx.unregisterRemoveInstrCallback(RemoveCbId); | ||
| Ctx.unregisterMoveInstrCallback(MoveCbId); | ||
| auto *NewI2 = sandboxir::StoreInst::create(Val, Ptr, /*Align=*/std::nullopt, | ||
| Ret->getIterator(), Ctx); | ||
| Ret->moveBefore(NewI2); | ||
| Ret->eraseFromParent(); | ||
| EXPECT_THAT(Inserted, testing::IsEmpty()); | ||
| EXPECT_THAT(Removed, testing::IsEmpty()); | ||
| EXPECT_THAT(Moved, testing::IsEmpty()); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use an ID and not the raw pointer of the callback itself as the ID ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-determinism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointers to the
std::functionobjects stored in a map/vector can get invalidated when registering/unregistering more callbacks, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to get non-determinism ? The ID itself is not printed anywhere, it's just used as a key to remove the callback if needed. If it's the raw pointer of the function it should still work just as well. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True but we can get around that in multiple ways, one of which is by allocating unique pointers and getting the raw pointer.
What I don't like about the integer IDs is that:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is another layer of indirection and an extra heap allocation better than keeping an integer around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use raw pointers as keys in maps, you will get non-determinism.
The context should own the counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's definitely not great, and we should try to make the code that runs the callbacks as fast as possible.
Since @tschuett brought up non-determinism, should we try to make sure that the callbacks are run in the same order as registered? Because we are currently not only not getting that order but since we are iterating over the DenseMap we are also getting a non-deterministic order. Using a MapVector will fix non-determinism, but perhaps we should try to stick to registration order, and try to change it later if it turns out that it's not needed. I think we could use a vector instead of a map, because removal of a callback shouldn't happen too often (and we won't have too many callbacks) so a linear-time search should be fine. Wdyt?