From b266ea739a7678804c14c4752dbfd8f7d1beeebe Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Fri, 8 Nov 2024 18:58:59 -0800 Subject: [PATCH 1/6] [SandboxIR] Preserve the order of switch cases after revert. Preserving the case order is not strictly necessary to preserve semantics (for example, operations like SwitchInst::removeCase will happily swap cases around). However, I'm planning to introduce an optional verification step for SandboxIR that will use StructuralHash to compare IR after a revert to the original IR to help catch tracker bugs, and the order difference triggers a difference there. --- llvm/include/llvm/SandboxIR/Tracker.h | 6 ++++-- llvm/lib/SandboxIR/Instruction.cpp | 2 +- llvm/lib/SandboxIR/Tracker.cpp | 14 +++++++++++++- llvm/unittests/SandboxIR/TrackerTest.cpp | 13 +++++++++++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/llvm/include/llvm/SandboxIR/Tracker.h b/llvm/include/llvm/SandboxIR/Tracker.h index 3e3e539a8c7c1..7db0fc23c75fb 100644 --- a/llvm/include/llvm/SandboxIR/Tracker.h +++ b/llvm/include/llvm/SandboxIR/Tracker.h @@ -315,12 +315,14 @@ class SwitchAddCase : public IRChangeBase { class SwitchRemoveCase : public IRChangeBase { SwitchInst *Switch; + unsigned Index; ConstantInt *Val; BasicBlock *Dest; public: - SwitchRemoveCase(SwitchInst *Switch, ConstantInt *Val, BasicBlock *Dest) - : Switch(Switch), Val(Val), Dest(Dest) {} + SwitchRemoveCase(SwitchInst *Switch, unsigned Index, ConstantInt *Val, + BasicBlock *Dest) + : Switch(Switch), Index(Index), Val(Val), Dest(Dest) {} void revert(Tracker &Tracker) final; void accept() final {} #ifndef NDEBUG diff --git a/llvm/lib/SandboxIR/Instruction.cpp b/llvm/lib/SandboxIR/Instruction.cpp index df941b2fa81ef..2df51a5947c17 100644 --- a/llvm/lib/SandboxIR/Instruction.cpp +++ b/llvm/lib/SandboxIR/Instruction.cpp @@ -1133,7 +1133,7 @@ void SwitchInst::addCase(ConstantInt *OnVal, BasicBlock *Dest) { SwitchInst::CaseIt SwitchInst::removeCase(CaseIt It) { auto &Case = *It; Ctx.getTracker().emplaceIfTracking( - this, Case.getCaseValue(), Case.getCaseSuccessor()); + this, Case.getCaseIndex(), Case.getCaseValue(), Case.getCaseSuccessor()); auto *LLVMSwitch = cast(Val); unsigned CaseNum = It - case_begin(); diff --git a/llvm/lib/SandboxIR/Tracker.cpp b/llvm/lib/SandboxIR/Tracker.cpp index abcad39330094..a6f25670c5520 100644 --- a/llvm/lib/SandboxIR/Tracker.cpp +++ b/llvm/lib/SandboxIR/Tracker.cpp @@ -170,7 +170,19 @@ void CatchSwitchAddHandler::revert(Tracker &Tracker) { LLVMCSI->removeHandler(LLVMCSI->handler_begin() + HandlerIdx); } -void SwitchRemoveCase::revert(Tracker &Tracker) { Switch->addCase(Val, Dest); } +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); +} #ifndef NDEBUG void SwitchRemoveCase::dump() const { diff --git a/llvm/unittests/SandboxIR/TrackerTest.cpp b/llvm/unittests/SandboxIR/TrackerTest.cpp index 6d060854949e1..71c4c3d0ea921 100644 --- a/llvm/unittests/SandboxIR/TrackerTest.cpp +++ b/llvm/unittests/SandboxIR/TrackerTest.cpp @@ -963,6 +963,19 @@ define void @foo(i32 %cond0, i32 %cond1) { EXPECT_EQ(Switch->getNumCases(), 2u); EXPECT_EQ(Switch->findCaseDest(BB0), Zero); EXPECT_EQ(Switch->findCaseDest(BB1), One); + + // Check order is preserved after reverting multiple removeCase invocations. + Ctx.save(); + Switch->removeCase(Switch->findCaseValue(Zero)); + Switch->removeCase(Switch->findCaseValue(One)); + EXPECT_EQ(Switch->getNumCases(), 0u); + Ctx.revert(); + EXPECT_EQ(Switch->getNumCases(), 2u); + EXPECT_EQ(Switch->findCaseDest(BB0), Zero); + EXPECT_EQ(Switch->findCaseDest(BB1), One); + EXPECT_EQ(Switch->getSuccessor(0), OrigDefaultDest); + EXPECT_EQ(Switch->getSuccessor(1), BB0); + EXPECT_EQ(Switch->getSuccessor(2), BB1); } TEST_F(TrackerTest, SelectInst) { From 6bccc01ac4e34dabd081d1c4dcfbc50ae616856f Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Mon, 11 Nov 2024 14:06:47 -0800 Subject: [PATCH 2/6] Added a separate "switch preserves successor order" test. The new test also tests a couple more situations, as requested during review. --- llvm/unittests/SandboxIR/TrackerTest.cpp | 76 +++++++++++++++++++++++- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/llvm/unittests/SandboxIR/TrackerTest.cpp b/llvm/unittests/SandboxIR/TrackerTest.cpp index 71c4c3d0ea921..077219b52260d 100644 --- a/llvm/unittests/SandboxIR/TrackerTest.cpp +++ b/llvm/unittests/SandboxIR/TrackerTest.cpp @@ -964,18 +964,88 @@ define void @foo(i32 %cond0, i32 %cond1) { EXPECT_EQ(Switch->findCaseDest(BB0), Zero); EXPECT_EQ(Switch->findCaseDest(BB1), One); - // Check order is preserved after reverting multiple removeCase invocations. +} + +TEST_F(TrackerTest, SwitchInstPreservesSuccesorOrder) { + parseIR(C, R"IR( +define void @foo(i32 %cond0) { + entry: + switch i32 %cond0, label %default [ i32 0, label %bb0 + i32 1, label %bb1 + i32 2, label %bb2 ] + bb0: + ret void + bb1: + ret void + bb2: + ret void + default: + ret void +} +)IR"); + Function &LLVMF = *M->getFunction("foo"); + auto *LLVMEntry = getBasicBlockByName(LLVMF, "entry"); + + sandboxir::Context Ctx(C); + [[maybe_unused]] auto &F = *Ctx.createFunction(&LLVMF); + auto *Entry = cast(Ctx.getValue(LLVMEntry)); + auto *BB0 = cast( + Ctx.getValue(getBasicBlockByName(LLVMF, "bb0"))); + auto *BB1 = cast( + Ctx.getValue(getBasicBlockByName(LLVMF, "bb1"))); + auto *BB2 = cast( + Ctx.getValue(getBasicBlockByName(LLVMF, "bb2"))); + auto *Switch = cast(&*Entry->begin()); + + auto *DefaultDest = Switch->getDefaultDest(); + auto *Zero = sandboxir::ConstantInt::get(sandboxir::Type::getInt32Ty(Ctx), 0); + auto *One = sandboxir::ConstantInt::get(sandboxir::Type::getInt32Ty(Ctx), 1); + auto *Two = sandboxir::ConstantInt::get(sandboxir::Type::getInt32Ty(Ctx), 2); + + // Check that we can properly revert a removeCase multiple positions apart + // from the end of the operand list. Ctx.save(); Switch->removeCase(Switch->findCaseValue(Zero)); + EXPECT_EQ(Switch->getNumCases(), 2u); + Ctx.revert(); + EXPECT_EQ(Switch->getNumCases(), 3u); + EXPECT_EQ(Switch->findCaseDest(BB0), Zero); + EXPECT_EQ(Switch->findCaseDest(BB1), One); + EXPECT_EQ(Switch->findCaseDest(BB2), Two); + EXPECT_EQ(Switch->getSuccessor(0), DefaultDest); + EXPECT_EQ(Switch->getSuccessor(1), BB0); + EXPECT_EQ(Switch->getSuccessor(2), BB1); + EXPECT_EQ(Switch->getSuccessor(3), BB2); + + // Check that we can properly revert a removeCase of the last case. + Ctx.save(); + Switch->removeCase(Switch->findCaseValue(Two)); + EXPECT_EQ(Switch->getNumCases(), 2u); + Ctx.revert(); + EXPECT_EQ(Switch->getNumCases(), 3u); + EXPECT_EQ(Switch->findCaseDest(BB0), Zero); + EXPECT_EQ(Switch->findCaseDest(BB1), One); + EXPECT_EQ(Switch->findCaseDest(BB2), Two); + EXPECT_EQ(Switch->getSuccessor(0), DefaultDest); + EXPECT_EQ(Switch->getSuccessor(1), BB0); + EXPECT_EQ(Switch->getSuccessor(2), BB1); + EXPECT_EQ(Switch->getSuccessor(3), BB2); + + // Check order is preserved after reverting multiple removeCase invocations. + Ctx.save(); Switch->removeCase(Switch->findCaseValue(One)); + Switch->removeCase(Switch->findCaseValue(Zero)); + Switch->removeCase(Switch->findCaseValue(Two)); EXPECT_EQ(Switch->getNumCases(), 0u); Ctx.revert(); - EXPECT_EQ(Switch->getNumCases(), 2u); + EXPECT_EQ(Switch->getNumCases(), 3u); EXPECT_EQ(Switch->findCaseDest(BB0), Zero); EXPECT_EQ(Switch->findCaseDest(BB1), One); - EXPECT_EQ(Switch->getSuccessor(0), OrigDefaultDest); + EXPECT_EQ(Switch->findCaseDest(BB2), Two); + EXPECT_EQ(Switch->getSuccessor(0), DefaultDest); EXPECT_EQ(Switch->getSuccessor(1), BB0); EXPECT_EQ(Switch->getSuccessor(2), BB1); + EXPECT_EQ(Switch->getSuccessor(3), BB2); } TEST_F(TrackerTest, SelectInst) { From 120eb87812b3de37a49ae8f14a90dd8a8181a094 Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Mon, 11 Nov 2024 14:17:30 -0800 Subject: [PATCH 3/6] clang-format --- llvm/unittests/SandboxIR/TrackerTest.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/llvm/unittests/SandboxIR/TrackerTest.cpp b/llvm/unittests/SandboxIR/TrackerTest.cpp index 077219b52260d..4f2cfa6b06ecd 100644 --- a/llvm/unittests/SandboxIR/TrackerTest.cpp +++ b/llvm/unittests/SandboxIR/TrackerTest.cpp @@ -963,7 +963,6 @@ define void @foo(i32 %cond0, i32 %cond1) { EXPECT_EQ(Switch->getNumCases(), 2u); EXPECT_EQ(Switch->findCaseDest(BB0), Zero); EXPECT_EQ(Switch->findCaseDest(BB1), One); - } TEST_F(TrackerTest, SwitchInstPreservesSuccesorOrder) { From 17ea6d8da8b5a6474d52fc7165c11817e31fa4d8 Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Tue, 12 Nov 2024 12:20:56 -0800 Subject: [PATCH 4/6] Avoid depending on llvm::SwitchInst::removeCase impl details. --- llvm/include/llvm/SandboxIR/Tracker.h | 13 ++++++------ llvm/lib/SandboxIR/Instruction.cpp | 4 +--- llvm/lib/SandboxIR/Tracker.cpp | 29 +++++++++++++++++---------- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/llvm/include/llvm/SandboxIR/Tracker.h b/llvm/include/llvm/SandboxIR/Tracker.h index 7db0fc23c75fb..dab20eb809ba0 100644 --- a/llvm/include/llvm/SandboxIR/Tracker.h +++ b/llvm/include/llvm/SandboxIR/Tracker.h @@ -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 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 diff --git a/llvm/lib/SandboxIR/Instruction.cpp b/llvm/lib/SandboxIR/Instruction.cpp index 2df51a5947c17..0a7cd95124bb5 100644 --- a/llvm/lib/SandboxIR/Instruction.cpp +++ b/llvm/lib/SandboxIR/Instruction.cpp @@ -1131,9 +1131,7 @@ void SwitchInst::addCase(ConstantInt *OnVal, BasicBlock *Dest) { } SwitchInst::CaseIt SwitchInst::removeCase(CaseIt It) { - auto &Case = *It; - Ctx.getTracker().emplaceIfTracking( - this, Case.getCaseIndex(), Case.getCaseValue(), Case.getCaseSuccessor()); + Ctx.getTracker().emplaceIfTracking(this); auto *LLVMSwitch = cast(Val); unsigned CaseNum = It - case_begin(); diff --git a/llvm/lib/SandboxIR/Tracker.cpp b/llvm/lib/SandboxIR/Tracker.cpp index a6f25670c5520..bf3d525ab83cd 100644 --- a/llvm/lib/SandboxIR/Tracker.cpp +++ b/llvm/lib/SandboxIR/Tracker.cpp @@ -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 + // 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. + unsigned NumCases = Switch->getNumCases(); + for (unsigned I = 0; I < NumCases; ++I) { + Switch->removeCase(Switch->case_begin()); + } + for (auto &Case : Cases) { + Switch->addCase(Case.Val, Case.Dest); + } } #ifndef NDEBUG From 8a980b27393c70747082df7b7ac72e570b906a87 Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Tue, 12 Nov 2024 12:26:59 -0800 Subject: [PATCH 5/6] clang-format again --- llvm/lib/SandboxIR/Tracker.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/SandboxIR/Tracker.cpp b/llvm/lib/SandboxIR/Tracker.cpp index bf3d525ab83cd..11ff2a97e2257 100644 --- a/llvm/lib/SandboxIR/Tracker.cpp +++ b/llvm/lib/SandboxIR/Tracker.cpp @@ -170,8 +170,8 @@ void CatchSwitchAddHandler::revert(Tracker &Tracker) { LLVMCSI->removeHandler(LLVMCSI->handler_begin() + HandlerIdx); } -SwitchRemoveCase::SwitchRemoveCase(SwitchInst *Switch): Switch(Switch) { - for (const auto& C : Switch->cases()) { +SwitchRemoveCase::SwitchRemoveCase(SwitchInst *Switch) : Switch(Switch) { + for (const auto &C : Switch->cases()) { Cases.push_back({C.getCaseValue(), C.getCaseSuccessor()}); } } From 79bdeb80e1fa11a44256c2c1a1c27c244f022054 Mon Sep 17 00:00:00 2001 From: Jorge Gorbe Moya Date: Tue, 12 Nov 2024 12:52:38 -0800 Subject: [PATCH 6/6] address more review feedback --- llvm/lib/SandboxIR/Tracker.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/llvm/lib/SandboxIR/Tracker.cpp b/llvm/lib/SandboxIR/Tracker.cpp index 11ff2a97e2257..b7e8b64f6e844 100644 --- a/llvm/lib/SandboxIR/Tracker.cpp +++ b/llvm/lib/SandboxIR/Tracker.cpp @@ -171,24 +171,22 @@ void CatchSwitchAddHandler::revert(Tracker &Tracker) { } SwitchRemoveCase::SwitchRemoveCase(SwitchInst *Switch) : Switch(Switch) { - for (const auto &C : Switch->cases()) { + for (const auto &C : Switch->cases()) Cases.push_back({C.getCaseValue(), C.getCaseSuccessor()}); - } } void SwitchRemoveCase::revert(Tracker &Tracker) { - // llvm::SwitchInst doesn't give us any API to insert cases at a specific - // 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. + // SwitchInst::removeCase doesn't provide any guarantees about the order of + // cases after removal. 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. unsigned NumCases = Switch->getNumCases(); - for (unsigned I = 0; I < NumCases; ++I) { + for (unsigned I = 0; I < NumCases; ++I) Switch->removeCase(Switch->case_begin()); - } - for (auto &Case : Cases) { + for (auto &Case : Cases) Switch->addCase(Case.Val, Case.Dest); - } } #ifndef NDEBUG