[panstromek]

First step towards replacing Option<Terminator> with Terminator in BasicBlockData. See this thread for more info.

This is a slight restructuring of the custom mir parser that avoids creating dummy basic blocks that don't have terminators. More details in individual commits.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir
• compiler, mir expanded to 77 candidates
• Random selection from 20 candidates

[hanna-kruppe]

This looks correct, but I have two questions.

r? me @rustbot author

View changes since this review

[hanna-kruppe]

I'm not sure I understand what this checks / what "order of BBs in cleanup blocks" means. We're comparing bb = i, which we get from enumerate of block_defs, with the index in the body's basic block list. That should always be true, unless something sneakily pushes another basic block before this loop or inside parse_block_def. Maybe that's what you had in mind, but either way the comment needs to be clearer.

[panstromek]

Maybe a better comment would be just "to make sure that these line up correctly." or "to make sure we're adding the correct block id"

But I'll step back and explain the issue, because this is a variation of a problem that happens in multiple places that use terminator: None, and maybe you'll get a better idea for what you would want to see commented here.

This code parses the blocks from two sources in two passes - first there's a list of declarations, parsed by parse_block_decls. We just get block numbers and is_cleanup flags from those. And then there's this code which parses the blocks themselves. Even here, these two list already don't line up. The declaration list omits the entry block, but definition list includes it, that's why we add the empty is_cleanup: false during construction.

The original code only has one place that creates basic blocks and assigns their ids (the declaration parser), and then always refers to existing blocks by id. The new code has (kinda) two places that assign ids. One is in the declaration parser, and one is the definition parser here. The old code implicitly prevented situation where you parse the block definition into incorrect id, the new code ensures that with the assert.

That should always be true, unless something sneakily pushes another basic block before this loop or inside parse_block_def

Yes. I added the assert to avoid this code implicitly assuming that other parts of the pipeline don't add basic blocks in other places (and I've actually hit this assert during refactoring, that's how I found out the implicit basic block added during construction)

[hanna-kruppe]

Okay, I understand better now, thanks. I think I was also misguided by my knowledge of the surface syntax that mir! accepts. Some unorganized thoughts:

• It's not just the cleanup block data, block_map also has to match how we actually assign block indices. For example, you can name a (non-entry) block foo and references to block foo elsewhere will be resolved to the index of foo within the list of block declarations (+1 for implicit entry block). That's another reason why order of blocks in body has to match declaration order.
• I think this isn't entirely a new problem. Before, we had only one loop allocate blocks in body, but we still had the same implicit dependency on how these blocks get initialized in parse_block_defs. Trying to create too many blocks probably would've panicked with an index out of bounds, but initializing fewer than were declared could've gone silently wrong.
• While block def parsing currently separates "allocate block index in body" from "actually push the block" this does not seem inherent. We could instead call parse_block_def first, get back a BasicBlockData , push it into the body, and then use the index at which it was inserted to look up whether it's a cleanup block. That would remove the "block gets wrong is_cleanup" problem, but won't do anything to ensure the block is created at the right index w.r.t. block_map.
• It seems more of a coincidence that the declaration phase "allocates block indices" by pushing to the cleanup_blocks vector.
  • We just happen to have another IndexVec indexed by basic blocks at hand, but in any other context, it wouldn't be the canonical source for which blocks exist.
  • Arguably we don't even need the "is cleanup" flag in the declarations and could instead associate it with the block definition.
  • I realize that we need to assign block indices somehow, to populate block_map if nothing else, and so using an IndexVec and populating it during declaration parsing kills two birds with one stone. But the more I think about it, the more it seems "too clever for its own good".

I now have some ideas for how this whole process could be refactored, but that's probably way out of scope for this PR. Instead I would propose:

1. At the end, assert that checks that all of cleanup_blocks, block_map, and the resulting body agree on the total number of blocks (block_map should have one less because the entry block is anonymous). That won't catch getting the order wrong, but the most likely way to mess up the order is by creating too extra blocks or skipping one.
2. For this comment, I'd say the main point for me is ensuring we create one block per block_def so that order and indices match the block declarations parsed above. Mentioning only cleanup without names here is misleading, but mentioning both together could be helpful.

[panstromek]

I now have some ideas for how this whole process could be refactored, but that's probably way out of scope for this PR.

Yea, I want to avoid another rabbit hole, so I'm just trying to find some minimal set of changes that I need for the Terminator change while preserving the original intent. I'll try to find some compromise

[hanna-kruppe]

Should this be a GrowableBitSet? The memory footprint doesn't matter, but it seems conceptually more appropriate than IndexVec<_, bool>.

[panstromek]

Maybe, I was thinking about it, but I wanted to preserve the hidden assumptions from the original code - this code not only tracks the cleanup flags, but it also allocates basic block ids with this vector.

If there's fewer block declarations then block definitions, accessing this vec will panic when block definitions are parsed. GrowableBitSet wouldn't panic.

[hanna-kruppe]

As mentioned in the other comment I just posted, I think the use of this IndexVec to allocate block indices is too clever. I'm fine with leaving it alone in this PR, but maybe add a comment explaining this dual purpose?

[panstromek]

I think I could just name it block_decls actually, that match the original code more than this name.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[panstromek]

oof, this was a bit tough to think through, but I hope the result is clearer now (even though the diff is tiny).

@rustbot ready

[hanna-kruppe]

Yeah, this turned out much more complicated than I expected. Thank you for explaining and sorting it out, I like the result!

(Well, I’m still tempted to make follow up changes to simplify this code some more, but that’s not your problem.)

@bors r+ rollup

[rust-bors]

📌 Commit c84c4c8 has been approved by hanna-kruppe

It is now in the queue for this repository.

[panstromek]

(Well, I’m still tempted to make follow up changes to simplify this code some more, but that’s not your problem.)

Totally go for it if you want. I'm just trying to stay on the task, I only did the bare minimum that I needed here ;)