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

slackito · 2024-11-09T03:21:48Z

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.

vporpo · 2024-11-09T16:53:45Z

llvm/lib/SandboxIR/Tracker.cpp

+  unsigned NumOps = Switch->getNumOperands();
+  auto ValUseB = Switch->getOperandUse(NumOps - 2);
+  auto SucUseB = Switch->getOperandUse(NumOps - 2 + 1);
+  ValUseA.swap(ValUseB);


I think this would require a loop that swaps the last 'case' one position to the left at a time until we reach Index. Because if the switch contains 3 cases (Case0, Case1, Case2) and we remove case 0, then when we revert we are placing the removed one at the end (Case1, Case2, Case0). Then we would need to bring Case0 back to position 0 without changing the order of 1 and 2.

Also I think we could use swapOperands() instead of swap(Use), which may be a bit more compact.

If I understand the implementation correctly, if you remove Case0 from (Case0, Case1, Case2), it swaps the element to be deleted with the last one, and then shrinks the list. So you actually get (Case2, Case1), which you can undo with a single swap after adding the case back at the end of the list.

Oh I hadn't realized that this is how it works, makes sense though. Hmm but should we rely on this behavior ? it seems too implementation specific, so if someone changes the implementation in LLVM IR, our tests will break. I think a comment that explains how this works would be really helpful.

The alternative would be to record the order of all cases when we create the change object. Then when we want to revert we remove them all and add them in again. Wdyt?

I think a comment that explains how this works would be really helpful.

That's what this comment a couple of lines above was supposed to be about:

// 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.

I can try to rephrase/expand it if it wasn't clear enough.

Hmm but should we rely on this behavior ? it seems too implementation specific, so if someone changes the implementation in LLVM IR, our tests will break.

Yeah, I was assuming the implementation wasn't very likely to change and that we would be fine updating this logic if it ever happens. If we don't feel comfortable with that option I can go with the alternative you suggested (recording all cases, then adding them again on revert) instead. Which one do you prefer?

I can try to rephrase/expand it if it wasn't clear enough.

Sorry, what I wanted to say is that it would be nice to show the example with the ordering of cases by actually showing them too like with (case0, case1, case2).

Which one do you prefer?

I am fine with either, but if we stick with this one, we should probably be very specific about the fact that this is implementation dependent and that it could break.

vporpo · 2024-11-09T16:56:02Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+  EXPECT_EQ(Switch->findCaseDest(BB1), One);
+  EXPECT_EQ(Switch->getSuccessor(0), OrigDefaultDest);
+  EXPECT_EQ(Switch->getSuccessor(1), BB0);
+  EXPECT_EQ(Switch->getSuccessor(2), BB1);


Please add a test where we remove case 0 of a 3-case switch, to check that we revert correctly in that case too.

tschuett · 2024-11-09T17:07:54Z

Could please add a SandBoxIR label to
https://github.com/llvm/llvm-project/blob/main/.github/new-prs-labeler.yml

The new test also tests a couple more situations, as requested during review.

github-actions · 2024-11-11T22:12:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

slackito · 2024-11-11T22:20:25Z

Could please add a SandBoxIR label to https://github.com/llvm/llvm-project/blob/main/.github/new-prs-labeler.yml

Sure, I'll send you a separate PR. Thanks for pointing it out.

vporpo · 2024-11-11T23:27:51Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+  EXPECT_EQ(Switch->getNumCases(), 0u);
+  Ctx.revert();
+  EXPECT_EQ(Switch->getNumCases(), 3u);
+  EXPECT_EQ(Switch->findCaseDest(BB0), Zero);


Perhaps this would be a good place to add a comment that if these checks fail, then it might be that the implementation in LLVM changed and that we should update the way revert() works.

Changed the implementation so it doesn't rely on the specific ordering changes introduced by removeCase()

vporpo · 2024-11-12T20:34:25Z

This solution is still implementation dependent given the lack of an addCase() API call with index. Not sure if I prefer this one over the previous one, given that the previous one was a constant-time operation.

vporpo · 2024-11-12T20:29:17Z

llvm/lib/SandboxIR/Tracker.cpp

-  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


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

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.

vporpo · 2024-11-12T20:29:32Z

llvm/lib/SandboxIR/Tracker.cpp

+  // 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) {


nit: drop the curly braces?

Done. Thanks!

vporpo · 2024-11-12T20:30:47Z

llvm/lib/SandboxIR/Tracker.cpp

+  // 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.


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

slackito · 2024-11-12T20:58:34Z

This solution is still implementation dependent given the lack of an addCase() API call with index. Not sure if I prefer this one over the previous one, given that the previous one was a constant-time operation.

I'm not super happy about it either, but I think this one is more robust against potential (if unlikely) removeCase changes.

If this turns out to be performance-sensitive we can go back to the other solution, and maybe open an RFC to document the existing behavior so we can rely on it.

As requested in #115577 (comment)

slackito requested review from alinas, vporpo, tschuett, aeubanks, tmsri and Sterling-Augustine November 9, 2024 03:21

vporpo reviewed Nov 9, 2024

View reviewed changes

Added a separate "switch preserves successor order" test.

6bccc01

The new test also tests a couple more situations, as requested during review.

clang-format

120eb87

vporpo reviewed Nov 11, 2024

View reviewed changes

slackito added 2 commits November 12, 2024 12:20

Avoid depending on llvm::SwitchInst::removeCase impl details.

17ea6d8

clang-format again

8a980b2

vporpo approved these changes Nov 12, 2024

View reviewed changes

address more review feedback

79bdeb8

slackito merged commit 9d85ba5 into llvm:main Nov 12, 2024
6 of 7 checks passed

slackito deleted the sandboxir-preserve-switchinst-operand-order branch November 12, 2024 23:02

slackito mentioned this pull request Nov 13, 2024

Add new "llvm:SandboxIR" label to .github/new-prs-labeler.yml #115965

Merged

slackito added a commit that referenced this pull request Nov 13, 2024

Add new "llvm:SandboxIR" label to .github/new-prs-labeler.yml (#115965)

48e09fe

As requested in #115577 (comment)

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

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

Uh oh!

Conversation

slackito commented Nov 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschuett commented Nov 9, 2024

Uh oh!

github-actions bot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slackito commented Nov 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vporpo commented Nov 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slackito commented Nov 12, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 11, 2024 •

edited

Loading