Avoid recursion in InstNamer::CollectNamesInBlock

[danakj]

Use a deque to maintain the set of instructions to be walked over. so that the loop can append more instructions (with their related scope) during iteration without requiring recursion.

[danakj]

I used a std::deque here cuz it makes sense at a high level, but maybe there's some more optimal data structure in the llvm project?

[danakj]

I have not figured out how these suffix numbers come to be, but the non-recursive walk is slightly differently ordered, in that it queues the recursion step instead of starting it immediately. Not sure if this is good/bad. I modified it to insert the recursive stuff right after the current instruction (at the front of the queue) instead of at the end. That is still slightly different as it would be queued after the current one instead of completing before it. And the numbering is still adjusted a bit, and I am not sure why yet...

[danakj]

(If you would like me to figure out why they are different here let me know and I will keep looking, and try to avoid that change, it's not clear to me if this matters or what)

[jonmeow]

TBH this looks like an improvement since the indices increase as code progresses. I suspect this is the break; after SpliceBlock in action, if that helps.

[danakj]

Yes, you're right. To preserve it we'd need to re-queue the current SpliceBlock instruction first before queueing the block id, in such a way that we'd then name the current SpliceBlock instruction after it, without also redoing the block id again infinitely.

But since you like the change I will leave it.

[jonmeow]

Regarding the use of deque, had you considered instead using SmallVector? I think that just requires adding in reverse order, such as:

Suggested change

	std::deque<std::pair<ScopeId, InstId>> insts;
	// Appends a scope and instructions to walk. Avoids recursion while allowing
	// the loop to below append more instructions during iteration.
	auto append_block_id = [&](ScopeId scope_id, InstBlockId block_id) {
	if (block_id.is_valid()) {
	for (auto inst_id : sem_ir_->inst_blocks().Get(block_id)) {
	insts.emplace_back(scope_id, inst_id);
	}
	}
	};
	auto append_block_insts = [&](ScopeId scope_id,
	llvm::ArrayRef<InstId> inst_ids) {
	for (auto inst_id : inst_ids) {
	insts.emplace_back(scope_id, inst_id);
	}
	};
	llvm::SmallVector<std::pair<ScopeId, InstId>> insts;
	// Appends a scope and instructions to walk. Avoids recursion while allowing
	// the loop to below append more instructions during iteration.
	auto append_block_id = [&](ScopeId scope_id, InstBlockId block_id) {
	if (block_id.is_valid()) {
	for (auto inst_id : llvm::reverse(sem_ir_->inst_blocks().Get(block_id))) {
	insts.emplace_back(scope_id, inst_id);
	}
	}
	};
	auto append_block_insts = [&](ScopeId scope_id,
	llvm::ArrayRef<InstId> inst_ids) {
	for (auto inst_id : llvm::reverse(inst_ids)) {
	insts.emplace_back(scope_id, inst_id);
	}
	};

[jonmeow]

Note: my understanding is that it'd probably be better to switch to push_back instead of emplace_back too (#4651 (review), #4651 (comment))

[zygoloid]

Switching from deque to a vector will switch us from FIFO to LIFO traversal -- that is, from breadth-first to depth-first -- which should keep the instruction numbering more predictable and closer to how it currently is. (We'll still have some differences because we're switching from in-order to pre-order traversal so we'll number later instructions in the parent block before we number the child block.)

[jonmeow]

Just to note, this'd switched from emplace_back/pop_front to emplace_front/pop_front (using llvm::reverse similar to what I'd suggested here). Although that means push_back/pop_back won't change traversal, it also means the deque's FIFO feature isn't used, making a vector more appealing.

[danakj]

Note: my understanding is that it'd probably be better to switch to push_back instead of emplace_back too (#4651 (review), #4651 (comment))

Ah, I didn't think that conclusion was so general, and pair is the poster child of emplace, but I would personally be happy to never write emplace again. :)

Originally I was pushing to the back, and popping from the front, but I changed that, so yeah a vector makes a lot more sense now, thanks for noticing that.

[jonmeow]

TBH this looks like an improvement since the indices increase as code progresses. I suspect this is the break; after SpliceBlock in action, if that helps.

[danakj]

PTAL :)

[jonmeow]

Thanks!