Skip to content

Conversation

@aengelke
Copy link
Contributor

@aengelke aengelke commented Nov 6, 2025

SwitchInst case values must be ConstantInt, which have no use list. Therefore it is not necessary to store these as Use, instead store them more efficiently as a simple array of pointers after the uses, similar to how PHINode stores basic blocks.

After this change, the successors of all terminators are stores consecutively in the operand list. This is preparatory work for improving the performance of successor access.

WIP: SandboxIR is broken.

cc @vporpo for SandboxIR
cc @nikic

Other things to consider:

  • C API to access case values
  • Release notes (this has breaking potential)

SwitchInst case values must be ConstantInt, which have no use list.
Therefore it is not necessary to store these as Use, instead store them
more efficiently as a simple array of pointers after the uses, similar
to how PHINode stores basic blocks.

After this change, the successors of all terminators are stores
consecutively in the operand list. This is preparatory work for
improving the performance of successor access.

WIP: SandboxIR is broken.
@vporpo
Copy link

vporpo commented Nov 7, 2025

That's a very interesting case @aengelke , thanks for sharing! Let's start with the obvious: the tests are crashing because of the nullptr in the line you added: ConstantInt *const *case_values() const { return nullptr; } // XXX, which you obviously had to add because otherwise it wouldn't build.
Well, having to do in the first place is against the design principles of Sandbox IR: LLVM's SwitchInst::case_values() function is private, not public, so Sandbox IR shouldn't interfere with it at all.
So why is this happening? Well it's because for the CaseHandle of the Sandbox IR SwitchInst we used the innocent-looking CaseHandleImpl template of LLVM IR's SwitchInst instead of creating a separate class, as is common practice in most other Sandbox IR Instructions. But this template is making assumptions about the private interface of SwitchInst (at least it does so with your changes), which creates a tight-coupling, a dependency cycle back to Sandbox IR which shouldn't be there in the first place.

The fix is quite simple. Similarly to most other components, Sandbox IR's CaseHandle (and most probably the case iterator too) should become its own separate class that depends on LLVM IR' public API and remove the dependency back to Sandbox IR. I will follow-up with a patch that fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants