From 569feab0f415d187b778e2e42354258adf6f550d Mon Sep 17 00:00:00 2001 From: Vasileios Porpodas Date: Fri, 9 Aug 2024 12:02:05 -0700 Subject: [PATCH] fixup! fixup! [SandboxIR] Clean up tracking code with the help of tryTrack() --- llvm/include/llvm/SandboxIR/Tracker.h | 135 +++++++++----------------- llvm/lib/SandboxIR/SandboxIR.cpp | 17 ++-- llvm/lib/SandboxIR/Tracker.cpp | 73 +++++--------- 3 files changed, 81 insertions(+), 144 deletions(-) diff --git a/llvm/include/llvm/SandboxIR/Tracker.h b/llvm/include/llvm/SandboxIR/Tracker.h index c95af75cfa5a2b..dea09f547b0be4 100644 --- a/llvm/include/llvm/SandboxIR/Tracker.h +++ b/llvm/include/llvm/SandboxIR/Tracker.h @@ -63,20 +63,15 @@ class AllocaInst; /// The base class for IR Change classes. class IRChangeBase { protected: - Tracker &Parent; + friend class Tracker; // For Parent. public: - IRChangeBase(Tracker &Parent); /// This runs when changes get reverted. - virtual void revert() = 0; + virtual void revert(Tracker &Tracker) = 0; /// This runs when changes get accepted. virtual void accept() = 0; virtual ~IRChangeBase() = default; #ifndef NDEBUG - /// \Returns the index of this change by iterating over all changes in the - /// tracker. This is only used for debugging. - unsigned getIdx() const; - void dumpCommon(raw_ostream &OS) const { OS << getIdx() << ". "; } virtual void dump(raw_ostream &OS) const = 0; LLVM_DUMP_METHOD virtual void dump() const = 0; friend raw_ostream &operator<<(raw_ostream &OS, const IRChangeBase &C) { @@ -92,15 +87,11 @@ class UseSet : public IRChangeBase { Value *OrigV = nullptr; public: - UseSet(const Use &U, Tracker &Tracker) - : IRChangeBase(Tracker), U(U), OrigV(U.get()) {} - void revert() final { U.set(OrigV); } + UseSet(const Use &U) : U(U), OrigV(U.get()) {} + void revert(Tracker &Tracker) final { U.set(OrigV); } void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "UseSet"; - } + void dump(raw_ostream &OS) const final { OS << "UseSet"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -115,14 +106,11 @@ class PHISetIncoming : public IRChangeBase { Value, Block, }; - PHISetIncoming(PHINode &PHI, unsigned Idx, What What, Tracker &Tracker); - void revert() final; + PHISetIncoming(PHINode &PHI, unsigned Idx, What What); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "PHISetIncoming"; - } + void dump(raw_ostream &OS) const final { OS << "PHISetIncoming"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -134,14 +122,11 @@ class PHIRemoveIncoming : public IRChangeBase { BasicBlock *RemovedBB; public: - PHIRemoveIncoming(PHINode &PHI, unsigned RemovedIdx, Tracker &Tracker); - void revert() final; + PHIRemoveIncoming(PHINode &PHI, unsigned RemovedIdx); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "PHISetIncoming"; - } + void dump(raw_ostream &OS) const final { OS << "PHISetIncoming"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -151,14 +136,11 @@ class PHIAddIncoming : public IRChangeBase { unsigned Idx; public: - PHIAddIncoming(PHINode &PHI, Tracker &Tracker); - void revert() final; + PHIAddIncoming(PHINode &PHI); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "PHISetIncoming"; - } + void dump(raw_ostream &OS) const final { OS << "PHISetIncoming"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -169,17 +151,14 @@ class UseSwap : public IRChangeBase { Use OtherUse; public: - UseSwap(const Use &ThisUse, const Use &OtherUse, Tracker &Tracker) - : IRChangeBase(Tracker), ThisUse(ThisUse), OtherUse(OtherUse) { + UseSwap(const Use &ThisUse, const Use &OtherUse) + : ThisUse(ThisUse), OtherUse(OtherUse) { assert(ThisUse.getUser() == OtherUse.getUser() && "Expected same user!"); } - void revert() final { ThisUse.swap(OtherUse); } + void revert(Tracker &Tracker) final { ThisUse.swap(OtherUse); } void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "UseSwap"; - } + void dump(raw_ostream &OS) const final { OS << "UseSwap"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -203,14 +182,11 @@ class EraseFromParent : public IRChangeBase { std::unique_ptr ErasedIPtr; public: - EraseFromParent(std::unique_ptr &&IPtr, Tracker &Tracker); - void revert() final; + EraseFromParent(std::unique_ptr &&IPtr); + void revert(Tracker &Tracker) final; void accept() final; #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "EraseFromParent"; - } + void dump(raw_ostream &OS) const final { OS << "EraseFromParent"; } LLVM_DUMP_METHOD void dump() const final; friend raw_ostream &operator<<(raw_ostream &OS, const EraseFromParent &C) { C.dump(OS); @@ -226,15 +202,12 @@ class RemoveFromParent : public IRChangeBase { PointerUnion NextInstrOrBB; public: - RemoveFromParent(Instruction *RemovedI, Tracker &Tracker); - void revert() final; + RemoveFromParent(Instruction *RemovedI); + void revert(Tracker &Tracker) final; void accept() final {}; Instruction *getInstruction() const { return RemovedI; } #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "RemoveFromParent"; - } + void dump(raw_ostream &OS) const final { OS << "RemoveFromParent"; } LLVM_DUMP_METHOD void dump() const final; #endif // NDEBUG }; @@ -265,15 +238,11 @@ class GenericSetter final : public IRChangeBase { SavedValT OrigVal; public: - GenericSetter(InstrT *I, Tracker &Tracker) - : IRChangeBase(Tracker), I(I), OrigVal((I->*GetterFn)()) {} - void revert() final { (I->*SetterFn)(OrigVal); } + GenericSetter(InstrT *I) : I(I), OrigVal((I->*GetterFn)()) {} + void revert(Tracker &Tracker) final { (I->*SetterFn)(OrigVal); } void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "GenericSetter"; - } + void dump(raw_ostream &OS) const final { OS << "GenericSetter"; } LLVM_DUMP_METHOD void dump() const final { dump(dbgs()); dbgs() << "\n"; @@ -287,14 +256,11 @@ class CallBrInstSetIndirectDest : public IRChangeBase { BasicBlock *OrigIndirectDest; public: - CallBrInstSetIndirectDest(CallBrInst *CallBr, unsigned Idx, Tracker &Tracker); - void revert() final; + CallBrInstSetIndirectDest(CallBrInst *CallBr, unsigned Idx); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "CallBrInstSetIndirectDest"; - } + void dump(raw_ostream &OS) const final { OS << "CallBrInstSetIndirectDest"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -307,14 +273,11 @@ class MoveInstr : public IRChangeBase { PointerUnion NextInstrOrBB; public: - MoveInstr(sandboxir::Instruction *I, Tracker &Tracker); - void revert() final; + MoveInstr(sandboxir::Instruction *I); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "MoveInstr"; - } + void dump(raw_ostream &OS) const final { OS << "MoveInstr"; } LLVM_DUMP_METHOD void dump() const final; #endif // NDEBUG }; @@ -323,14 +286,11 @@ class InsertIntoBB final : public IRChangeBase { Instruction *InsertedI = nullptr; public: - InsertIntoBB(Instruction *InsertedI, Tracker &Tracker); - void revert() final; + InsertIntoBB(Instruction *InsertedI); + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "InsertIntoBB"; - } + void dump(raw_ostream &OS) const final { OS << "InsertIntoBB"; } LLVM_DUMP_METHOD void dump() const final; #endif // NDEBUG }; @@ -339,15 +299,11 @@ class CreateAndInsertInst final : public IRChangeBase { Instruction *NewI = nullptr; public: - CreateAndInsertInst(Instruction *NewI, Tracker &Tracker) - : IRChangeBase(Tracker), NewI(NewI) {} - void revert() final; + CreateAndInsertInst(Instruction *NewI) : NewI(NewI) {} + void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG - void dump(raw_ostream &OS) const final { - dumpCommon(OS); - OS << "CreateAndInsertInst"; - } + void dump(raw_ostream &OS) const final { OS << "CreateAndInsertInst"; } LLVM_DUMP_METHOD void dump() const final; #endif }; @@ -364,9 +320,6 @@ class Tracker { private: /// The list of changes that are being tracked. SmallVector> Changes; -#ifndef NDEBUG - friend unsigned IRChangeBase::getIdx() const; // For accessing `Changes`. -#endif /// The current state of the tracker. TrackerState State = TrackerState::Disabled; Context &Ctx; @@ -385,6 +338,12 @@ class Tracker { /// track Sandbox IR changes. void track(std::unique_ptr &&Change) { assert(State == TrackerState::Record && "The tracker should be tracking!"); +#ifndef NDEBUG + assert(!InMiddleOfCreatingChange && + "We are in the middle of creating another change!"); + if (isTracking()) + InMiddleOfCreatingChange = true; +#endif // NDEBUG Changes.push_back(std::move(Change)); #ifndef NDEBUG @@ -394,10 +353,10 @@ class Tracker { /// A convenience wrapper for `track()` that constructs and tracks the Change /// object if tracking is enabled. \Returns true if tracking is enabled. template - bool emplaceIfTracking(ArgsT... ChangeArgs) { + bool emplaceIfTracking(ArgsT... Args) { if (!isTracking()) return false; - track(std::make_unique(ChangeArgs..., *this)); + track(std::make_unique(Args...)); return true; } /// \Returns true if the tracker is recording changes. diff --git a/llvm/lib/SandboxIR/SandboxIR.cpp b/llvm/lib/SandboxIR/SandboxIR.cpp index 6fddd132fe9932..65dc8b5c128827 100644 --- a/llvm/lib/SandboxIR/SandboxIR.cpp +++ b/llvm/lib/SandboxIR/SandboxIR.cpp @@ -159,7 +159,7 @@ void Value::replaceAllUsesWith(Value *Other) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) { for (auto Use : uses()) - Tracker.track(std::make_unique(Use, Tracker)); + Tracker.track(std::make_unique(Use)); } // We are delegating RAUW to LLVM IR's RAUW. Val->replaceAllUsesWith(Other->Val); @@ -370,8 +370,7 @@ void Instruction::eraseFromParent() { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) { - Tracker.track( - std::make_unique(std::move(Detached), Tracker)); + Tracker.track(std::make_unique(std::move(Detached))); // We don't actually delete the IR instruction, because then it would be // impossible to bring it back from the dead at the same memory location. // Instead we remove it from its BB and track its current location. @@ -1128,7 +1127,7 @@ void PHINode::setIncomingValue(unsigned Idx, Value *V) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) Tracker.track(std::make_unique( - *this, Idx, PHISetIncoming::What::Value, Tracker)); + *this, Idx, PHISetIncoming::What::Value)); cast(Val)->setIncomingValue(Idx, V->Val); } @@ -1145,14 +1144,14 @@ void PHINode::setIncomingBlock(unsigned Idx, BasicBlock *BB) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) Tracker.track(std::make_unique( - *this, Idx, PHISetIncoming::What::Block, Tracker)); + *this, Idx, PHISetIncoming::What::Block)); cast(Val)->setIncomingBlock(Idx, cast(BB->Val)); } void PHINode::addIncoming(Value *V, BasicBlock *BB) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) - Tracker.track(std::make_unique(*this, Tracker)); + Tracker.track(std::make_unique(*this)); cast(Val)->addIncoming(V->Val, cast(BB->Val)); @@ -1160,7 +1159,7 @@ void PHINode::addIncoming(Value *V, BasicBlock *BB) { Value *PHINode::removeIncomingValue(unsigned Idx) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) - Tracker.track(std::make_unique(*this, Idx, Tracker)); + Tracker.track(std::make_unique(*this, Idx)); llvm::Value *LLVMV = cast(Val)->removeIncomingValue(Idx, /*DeletePHIIfEmpty=*/false); @@ -1169,8 +1168,8 @@ Value *PHINode::removeIncomingValue(unsigned Idx) { Value *PHINode::removeIncomingValue(BasicBlock *BB) { auto &Tracker = Ctx.getTracker(); if (Tracker.isTracking()) - Tracker.track(std::make_unique( - *this, getBasicBlockIndex(BB), Tracker)); + Tracker.track( + std::make_unique(*this, getBasicBlockIndex(BB))); auto *LLVMBB = cast(BB->Val); llvm::Value *LLVMV = diff --git a/llvm/lib/SandboxIR/Tracker.cpp b/llvm/lib/SandboxIR/Tracker.cpp index b4aea23cfc64d8..44fe0b7a212e03 100644 --- a/llvm/lib/SandboxIR/Tracker.cpp +++ b/llvm/lib/SandboxIR/Tracker.cpp @@ -15,22 +15,7 @@ using namespace llvm::sandboxir; -IRChangeBase::IRChangeBase(Tracker &Parent) : Parent(Parent) { #ifndef NDEBUG - assert(!Parent.InMiddleOfCreatingChange && - "We are in the middle of creating another change!"); - if (Parent.isTracking()) - Parent.InMiddleOfCreatingChange = true; -#endif // NDEBUG -} - -#ifndef NDEBUG -unsigned IRChangeBase::getIdx() const { - auto It = - find_if(Parent.Changes, [this](auto &Ptr) { return Ptr.get() == this; }); - return It - Parent.Changes.begin(); -} - void UseSet::dump() const { dump(dbgs()); dbgs() << "\n"; @@ -42,9 +27,8 @@ void UseSwap::dump() const { } #endif // NDEBUG -PHISetIncoming::PHISetIncoming(PHINode &PHI, unsigned Idx, What What, - Tracker &Tracker) - : IRChangeBase(Tracker), PHI(PHI), Idx(Idx) { +PHISetIncoming::PHISetIncoming(PHINode &PHI, unsigned Idx, What What) + : PHI(PHI), Idx(Idx) { switch (What) { case What::Value: OrigValueOrBB = PHI.getIncomingValue(Idx); @@ -55,7 +39,7 @@ PHISetIncoming::PHISetIncoming(PHINode &PHI, unsigned Idx, What What, } } -void PHISetIncoming::revert() { +void PHISetIncoming::revert(Tracker &Tracker) { if (auto *V = OrigValueOrBB.dyn_cast()) PHI.setIncomingValue(Idx, V); else @@ -69,14 +53,13 @@ void PHISetIncoming::dump() const { } #endif // NDEBUG -PHIRemoveIncoming::PHIRemoveIncoming(PHINode &PHI, unsigned RemovedIdx, - Tracker &Tracker) - : IRChangeBase(Tracker), PHI(PHI), RemovedIdx(RemovedIdx) { +PHIRemoveIncoming::PHIRemoveIncoming(PHINode &PHI, unsigned RemovedIdx) + : PHI(PHI), RemovedIdx(RemovedIdx) { RemovedV = PHI.getIncomingValue(RemovedIdx); RemovedBB = PHI.getIncomingBlock(RemovedIdx); } -void PHIRemoveIncoming::revert() { +void PHIRemoveIncoming::revert(Tracker &Tracker) { // Special case: if the PHI is now empty, as we don't need to care about the // order of the incoming values. unsigned NumIncoming = PHI.getNumIncomingValues(); @@ -105,10 +88,10 @@ void PHIRemoveIncoming::dump() const { } #endif // NDEBUG -PHIAddIncoming::PHIAddIncoming(PHINode &PHI, Tracker &Tracker) - : IRChangeBase(Tracker), PHI(PHI), Idx(PHI.getNumIncomingValues()) {} +PHIAddIncoming::PHIAddIncoming(PHINode &PHI) + : PHI(PHI), Idx(PHI.getNumIncomingValues()) {} -void PHIAddIncoming::revert() { PHI.removeIncomingValue(Idx); } +void PHIAddIncoming::revert(Tracker &Tracker) { PHI.removeIncomingValue(Idx); } #ifndef NDEBUG void PHIAddIncoming::dump() const { @@ -121,9 +104,8 @@ Tracker::~Tracker() { assert(Changes.empty() && "You must accept or revert changes!"); } -EraseFromParent::EraseFromParent(std::unique_ptr &&ErasedIPtr, - Tracker &Tracker) - : IRChangeBase(Tracker), ErasedIPtr(std::move(ErasedIPtr)) { +EraseFromParent::EraseFromParent(std::unique_ptr &&ErasedIPtr) + : ErasedIPtr(std::move(ErasedIPtr)) { auto *I = cast(this->ErasedIPtr.get()); auto LLVMInstrs = I->getLLVMInstrs(); // Iterate in reverse program order. @@ -151,7 +133,7 @@ void EraseFromParent::accept() { IData.LLVMI->deleteValue(); } -void EraseFromParent::revert() { +void EraseFromParent::revert(Tracker &Tracker) { // Place the bottom-most instruction first. auto [Operands, BotLLVMI] = InstrData[0]; if (auto *NextLLVMI = NextLLVMIOrBB.dyn_cast()) { @@ -170,7 +152,7 @@ void EraseFromParent::revert() { LLVMI->setOperand(OpNum, Op); BotLLVMI = LLVMI; } - Parent.getContext().registerValue(std::move(ErasedIPtr)); + Tracker.getContext().registerValue(std::move(ErasedIPtr)); } #ifndef NDEBUG @@ -180,15 +162,14 @@ void EraseFromParent::dump() const { } #endif // NDEBUG -RemoveFromParent::RemoveFromParent(Instruction *RemovedI, Tracker &Tracker) - : IRChangeBase(Tracker), RemovedI(RemovedI) { +RemoveFromParent::RemoveFromParent(Instruction *RemovedI) : RemovedI(RemovedI) { if (auto *NextI = RemovedI->getNextNode()) NextInstrOrBB = NextI; else NextInstrOrBB = RemovedI->getParent(); } -void RemoveFromParent::revert() { +void RemoveFromParent::revert(Tracker &Tracker) { if (auto *NextI = NextInstrOrBB.dyn_cast()) { RemovedI->insertBefore(NextI); } else { @@ -205,12 +186,11 @@ void RemoveFromParent::dump() const { #endif CallBrInstSetIndirectDest::CallBrInstSetIndirectDest(CallBrInst *CallBr, - unsigned Idx, - Tracker &Tracker) - : IRChangeBase(Tracker), CallBr(CallBr), Idx(Idx) { + unsigned Idx) + : CallBr(CallBr), Idx(Idx) { OrigIndirectDest = CallBr->getIndirectDest(Idx); } -void CallBrInstSetIndirectDest::revert() { +void CallBrInstSetIndirectDest::revert(Tracker &Tracker) { CallBr->setIndirectDest(Idx, OrigIndirectDest); } #ifndef NDEBUG @@ -220,15 +200,14 @@ void CallBrInstSetIndirectDest::dump() const { } #endif -MoveInstr::MoveInstr(Instruction *MovedI, Tracker &Tracker) - : IRChangeBase(Tracker), MovedI(MovedI) { +MoveInstr::MoveInstr(Instruction *MovedI) : MovedI(MovedI) { if (auto *NextI = MovedI->getNextNode()) NextInstrOrBB = NextI; else NextInstrOrBB = MovedI->getParent(); } -void MoveInstr::revert() { +void MoveInstr::revert(Tracker &Tracker) { if (auto *NextI = NextInstrOrBB.dyn_cast()) { MovedI->moveBefore(NextI); } else { @@ -244,10 +223,9 @@ void MoveInstr::dump() const { } #endif -void InsertIntoBB::revert() { InsertedI->removeFromParent(); } +void InsertIntoBB::revert(Tracker &Tracker) { InsertedI->removeFromParent(); } -InsertIntoBB::InsertIntoBB(Instruction *InsertedI, Tracker &Tracker) - : IRChangeBase(Tracker), InsertedI(InsertedI) {} +InsertIntoBB::InsertIntoBB(Instruction *InsertedI) : InsertedI(InsertedI) {} #ifndef NDEBUG void InsertIntoBB::dump() const { @@ -256,7 +234,7 @@ void InsertIntoBB::dump() const { } #endif -void CreateAndInsertInst::revert() { NewI->eraseFromParent(); } +void CreateAndInsertInst::revert(Tracker &Tracker) { NewI->eraseFromParent(); } #ifndef NDEBUG void CreateAndInsertInst::dump() const { @@ -271,7 +249,7 @@ void Tracker::revert() { assert(State == TrackerState::Record && "Forgot to save()!"); State = TrackerState::Disabled; for (auto &Change : reverse(Changes)) - Change->revert(); + Change->revert(*this); Changes.clear(); } @@ -285,7 +263,8 @@ void Tracker::accept() { #ifndef NDEBUG void Tracker::dump(raw_ostream &OS) const { - for (const auto &ChangePtr : Changes) { + for (auto [Idx, ChangePtr] : enumerate(Changes)) { + OS << Idx << ". "; ChangePtr->dump(OS); OS << "\n"; }