Move default blocks into jump tables#5756
Conversation
github-actions
bot
added
cranelift
elliottt
force-pushed
the
trevor/default-in-jump-table
branch
from
0a1e9ad to
aca99d1
Compare
|
I think it is less clear what the default case is for the br_table with this change. Maybe a JumpTableData could be formatted as edit: nevermind you are already doing that. I thought you hadn't updated the tests yet. my bad. |
| /// Create a new empty jump table. | ||
| pub fn new() -> Self { | ||
| Self { table: Vec::new() } | ||
| pub fn new(def: Block) -> Self { |
There was a problem hiding this comment.
This sounds like define or definition to me. Default is a keyword, and default_ isn't the nicest either. Unless someone has a better suggestion I guess leaving it the way it is right now is fine.
elliottt
force-pushed
the
trevor/default-in-jump-table
branch
from
90a9b38 to
dd61d04
Compare
| visit(inst, table.default_block(), false); | ||
|
|
||
| for &dest in f.stencil.dfg.jump_tables[*table].as_slice() { | ||
| for &dest in table.as_slice() { |
There was a problem hiding this comment.
I was going to say that this should just use table.all_branches() but then I remembered that would change the visit order. Instead, would you update the comment above to note that some callers depend on the default block being visited first?
| /// | ||
| /// The default block for the jump table is stored as the last element of the underlying vector, | ||
| /// and is not included in the length of the jump table. It can be accessed through the | ||
| /// `default_block` function. The default block is iterated via the `iter` and `iter_mut` methods. |
There was a problem hiding this comment.
This comment should be updated to refer to all_branches instead of iter.
3 participants
While working through #5731, adapting
branch_destinationto return an iterator over multiple slices ofBlockCallvalues was a little awkward, and yielded more complicated indexing for heavily used operations likeinst_values. As the root of the problem is the jump table default target and data not living in contiguous memory, there was a somewhat obvious solution: inline the default block into theJumpTableData, and remove it from theBranchTableinstruction data.The resulting refactoring is implemented in this PR: jump tables require a default block when they are created, and maintain an invariant that the last element of their internal vector is always the default label. Additionally, they now export slices for both the vector and just the jump table, to make it easy to adapt existing uses of
as_slice.For a little bit of additional motivation, consider this godbolt link. There are three indexing functions implemented: one that indexes into two slices using
nthon a chained iterator; one that checks which slice the index falls on; and one that indexes into a single slice. By far the best option is indexing into a single slice, followed by choosing the slice to index, followed by usingnth. Without this refactoring, we would need to pick the approach in eitherindex1orindex2to finish transitioningbr_tableto usingBlockCallarguments, and both feel like bad options.