-
Notifications
You must be signed in to change notification settings - Fork 14.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 all 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 |
---|---|---|
|
@@ -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,100 @@ 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.registerCreateInstrCallback( | ||
[&Inserted](sandboxir::Instruction *I) { Inserted.push_back(I); }); | ||
|
||
SmallVector<sandboxir::Instruction *> Removed; | ||
auto RemoveCbId = Ctx.registerEraseInstrCallback( | ||
[&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)); | ||
}); | ||
|
||
// Two more insertion callbacks, to check that they're called in registration | ||
// order. | ||
SmallVector<int> Order; | ||
auto CheckOrderInsertCbId1 = Ctx.registerCreateInstrCallback( | ||
[&Order](sandboxir::Instruction *I) { Order.push_back(1); }); | ||
|
||
auto CheckOrderInsertCbId2 = Ctx.registerCreateInstrCallback( | ||
[&Order](sandboxir::Instruction *I) { Order.push_back(2); }); | ||
|
||
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()); | ||
EXPECT_THAT(Order, testing::ElementsAre(1, 2)); | ||
|
||
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? 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))); | ||
EXPECT_THAT(Order, testing::ElementsAre(1, 2, 1, 2, 1, 2)); | ||
|
||
// Check that deregistration works. Do an operation of each type after | ||
// deregistering callbacks and check. | ||
Inserted.clear(); | ||
Removed.clear(); | ||
Moved.clear(); | ||
Ctx.unregisterCreateInstrCallback(InsertCbId); | ||
Ctx.unregisterEraseInstrCallback(RemoveCbId); | ||
Ctx.unregisterMoveInstrCallback(MoveCbId); | ||
Ctx.unregisterCreateInstrCallback(CheckOrderInsertCbId1); | ||
Ctx.unregisterCreateInstrCallback(CheckOrderInsertCbId2); | ||
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()); | ||
} |
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 about an assertion on
EraseInstrCallbacks.size()
checking that it's smaller than say 16 ? It could help catch issues where you keep on registering callbacks by mistake.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. I'm not super happy with having an arbitrary limit like this not documented anywhere and with a kinda low value, but it will be useful while we're developing the vectorizer and we can always revisit this decision later.