Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SandboxIR] Clean up tracking code with the help of tryTrack() #102406

Merged
merged 1 commit into from
Aug 9, 2024
Merged

[SandboxIR] Clean up tracking code with the help of tryTrack() #102406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/include/llvm/SandboxIR/SandboxIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ class Value {
template <typename ItTy, typename SBTy> friend class LLVMOpUserItToSBTy;

Value(ClassID SubclassID, llvm::Value *Val, Context &Ctx);
/// Disable copies.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried to use tryTrack() in PHINode::setIncomingValue() I realized that PHISetIncoming's constructor was taking a copy of the the instruction, rather than a reference, which would cause a crash when reverting. So I disabled copies here and I also reverted back to the original tracking code for setIncomingValue(). I will address those in a separate patch.

Value(const Value &) = delete;
Value &operator=(const Value &) = delete;

public:
virtual ~Value() = default;
Expand Down
149 changes: 62 additions & 87 deletions llvm/include/llvm/SandboxIR/Tracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
};
Expand All @@ -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
};
Expand All @@ -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
};
Expand All @@ -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
};
Expand All @@ -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
};
Expand All @@ -203,14 +182,11 @@ class EraseFromParent : public IRChangeBase {
std::unique_ptr<sandboxir::Value> ErasedIPtr;

public:
EraseFromParent(std::unique_ptr<sandboxir::Value> &&IPtr, Tracker &Tracker);
void revert() final;
EraseFromParent(std::unique_ptr<sandboxir::Value> &&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);
Expand All @@ -226,15 +202,12 @@ class RemoveFromParent : public IRChangeBase {
PointerUnion<Instruction *, BasicBlock *> 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
};
Expand Down Expand Up @@ -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";
Expand All @@ -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
};
Expand All @@ -307,14 +273,11 @@ class MoveInstr : public IRChangeBase {
PointerUnion<Instruction *, BasicBlock *> 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
};
Expand All @@ -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
};
Expand All @@ -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
};
Expand All @@ -364,9 +320,6 @@ class Tracker {
private:
/// The list of changes that are being tracked.
SmallVector<std::unique_ptr<IRChangeBase>> Changes;
#ifndef NDEBUG
friend unsigned IRChangeBase::getIdx() const; // For accessing `Changes`.
#endif
/// The current state of the tracker.
TrackerState State = TrackerState::Disabled;
Context &Ctx;
Expand All @@ -383,7 +336,29 @@ class Tracker {
Context &getContext() const { return Ctx; }
/// Record \p Change and take ownership. This is the main function used to
/// track Sandbox IR changes.
void track(std::unique_ptr<IRChangeBase> &&Change);
void track(std::unique_ptr<IRChangeBase> &&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
InMiddleOfCreatingChange = false;
#endif
}
/// A convenience wrapper for `track()` that constructs and tracks the Change
/// object if tracking is enabled. \Returns true if tracking is enabled.
template <typename ChangeT, typename... ArgsT>
bool emplaceIfTracking(ArgsT... Args) {
if (!isTracking())
return false;
track(std::make_unique<ChangeT>(Args...));
return true;
}
/// \Returns true if the tracker is recording changes.
bool isTracking() const { return State == TrackerState::Record; }
/// \Returns the current state of the tracker.
Expand Down
Loading
Loading