Skip to content

[SandboxIR] Preserve the order of switch cases after revert. #115577

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

Merged
merged 6 commits into from
Nov 12, 2024
Merged

[SandboxIR] Preserve the order of switch cases after revert. #115577

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b266ea7
[SandboxIR] Preserve the order of switch cases after revert.
slackito Nov 9, 2024
6bccc01
Added a separate "switch preserves successor order" test.
slackito Nov 11, 2024
120eb87
clang-format
slackito Nov 11, 2024
17ea6d8
Avoid depending on llvm::SwitchInst::removeCase impl details.
slackito Nov 12, 2024
8a980b2
clang-format again
slackito Nov 12, 2024
79bdeb8
address more review feedback
slackito Nov 12, 2024
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
13 changes: 7 additions & 6 deletions llvm/include/llvm/SandboxIR/Tracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,14 +315,15 @@ class SwitchAddCase : public IRChangeBase {

class SwitchRemoveCase : public IRChangeBase {
SwitchInst *Switch;
unsigned Index;
ConstantInt *Val;
BasicBlock *Dest;
struct Case {
ConstantInt *Val;
BasicBlock *Dest;
};
SmallVector<Case> Cases;

public:
SwitchRemoveCase(SwitchInst *Switch, unsigned Index, ConstantInt *Val,
BasicBlock *Dest)
: Switch(Switch), Index(Index), Val(Val), Dest(Dest) {}
SwitchRemoveCase(SwitchInst *Switch);

void revert(Tracker &Tracker) final;
void accept() final {}
#ifndef NDEBUG
Expand Down
4 changes: 1 addition & 3 deletions llvm/lib/SandboxIR/Instruction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,9 +1131,7 @@ void SwitchInst::addCase(ConstantInt *OnVal, BasicBlock *Dest) {
}

SwitchInst::CaseIt SwitchInst::removeCase(CaseIt It) {
auto &Case = *It;
Ctx.getTracker().emplaceIfTracking<SwitchRemoveCase>(
this, Case.getCaseIndex(), Case.getCaseValue(), Case.getCaseSuccessor());
Ctx.getTracker().emplaceIfTracking<SwitchRemoveCase>(this);

auto *LLVMSwitch = cast<llvm::SwitchInst>(Val);
unsigned CaseNum = It - case_begin();
Expand Down
29 changes: 18 additions & 11 deletions llvm/lib/SandboxIR/Tracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,18 +170,25 @@ void CatchSwitchAddHandler::revert(Tracker &Tracker) {
LLVMCSI->removeHandler(LLVMCSI->handler_begin() + HandlerIdx);
}

SwitchRemoveCase::SwitchRemoveCase(SwitchInst *Switch): Switch(Switch) {
for (const auto& C : Switch->cases()) {
Cases.push_back({C.getCaseValue(), C.getCaseSuccessor()});
}
}

void SwitchRemoveCase::revert(Tracker &Tracker) {
// removeCase swaps the last case with the deleted one. To revert it, we use
// addCase (which adds the new case to the end), and swap the newly-added
// value and successor operands to the positions for the original case index.
Switch->addCase(Val, Dest);
auto ValUseA = Switch->getOperandUse(2 + Index * 2);
auto SucUseA = Switch->getOperandUse(2 + Index * 2 + 1);
unsigned NumOps = Switch->getNumOperands();
auto ValUseB = Switch->getOperandUse(NumOps - 2);
auto SucUseB = Switch->getOperandUse(NumOps - 2 + 1);
ValUseA.swap(ValUseB);
SucUseA.swap(SucUseB);
// llvm::SwitchInst doesn't give us any API to insert cases at a specific
Copy link
Contributor

@vporpo vporpo Nov 12, 2024

Choose a reason for hiding this comment

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

Please also mention that removeCase() provides no guarantees about the order.

Copy link
Collaborator Author

@slackito slackito Nov 12, 2024

Choose a reason for hiding this comment

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

Done, even if we had APIs to insert cases at specific indices we'd still need to save everything to account for the lack of ordering guarantees in removeCase, so I've changed it to say just that.

// index. In order to preserve the original ordering, we save all of them and,
// when reverting, clear them all then insert them in the desired order. This
// still relies on the fact that `addCase` will insert them at the end, but it
// is documented to invalidate `case_end()` so it's probably okay.
Copy link
Contributor

@vporpo vporpo Nov 12, 2024

Choose a reason for hiding this comment

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

Hmm so this too is implementation dependent, but I guess less so?

unsigned NumCases = Switch->getNumCases();
for (unsigned I = 0; I < NumCases; ++I) {
Copy link
Contributor

@vporpo vporpo Nov 12, 2024

Choose a reason for hiding this comment

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

nit: drop the curly braces?

Copy link
Collaborator Author

@slackito slackito Nov 12, 2024

Choose a reason for hiding this comment

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

Done. Thanks!

Switch->removeCase(Switch->case_begin());
}
for (auto &Case : Cases) {
Switch->addCase(Case.Val, Case.Dest);
}
}

#ifndef NDEBUG
Expand Down
Loading