[TIR][Transform] Clear buffer_map during MakeUnpackedAPI

[Lunderberg]

This mimics the behavior in MakePackedAPI, and is assumed to be the case for some codegens.

[Mousius]

@Lunderberg I think we make use of this in following passes and I'm unsure why make packed API decides to clear it?

[Lunderberg]

@Mousius Thank you, and it looks like that's also being caught in the tests, as this breaks pretty much every ethos-u test at this line. (Also why I intentionally marked this as a draft PR, to catch any impact to workflows that I'm not as familiar with.)

To me, the buffer_map is an indicator for how a function should later be lowered, and what buffers should be defined as part of the lowered function. Once it has been used, then it makes sense for the resulting function to have an empty buffer_map, because no further lowering is required. After #9727, the buffer shape/size could still be accessed by later passes find finding the BufferLoad or BufferStore object that makes use of a parameter, rather than searching in the buffer_map itself, or the passes could be moved to an earlier stage of lowering.

[Lunderberg]

@Mousius Can you check the latest commit on this PR? I didn't try to re-order the passes, but reading from the BufferStore/BufferLoad objects instead of the buffer_map resolved the test cases I ran locally.

[Mousius]

To me, the buffer_map is an indicator for how a function should later be lowered, and what buffers should be defined as part of the lowered function. Once it has been used, then it makes sense for the resulting function to have an empty buffer_map, because no further lowering is required. After #9727, the buffer shape/size could still be accessed by later passes find finding the BufferLoad or BufferStore object that makes use of a parameter, rather than searching in the buffer_map itself, or the passes could be moved to an earlier stage of lowering.

Aha! Makes perfect sense, thanks for the explanation! I figured that was why it was in draft, being a bit nosey as I remember wondering about it when I was writing MakeUnpackedAPI 😸

[Mousius]

@Mousius Can you check the latest commit on this PR? I didn't try to re-order the passes, but reading from the BufferStore/BufferLoad objects instead of the buffer_map resolved the test cases I ran locally.

The fix makes sense to me, can we either make use of get_loads and get_stores or move the collect_buffer_map function to utils.py?

Also can we add to the documentation of the Pass the intention behind clearing it?

[Lunderberg]

The fix makes sense to me, can we either make use of get_loads and get_stores or move the collect_buffer_map function to utils.py?

Certainly. It is now pulled out and documented, with defined behavior for buffers that share a backing Var.

[Lunderberg]

I figured that was why it was in draft, being a bit nosey as I remember wondering about it when I was writing MakeUnpackedAPI 😸

No worries at all, and I wouldn't consider it nosey, either. It's the exact type of comment I was hoping for, as there wasn't an immediately obvious reason for the asymmetry between packed and unpacked.

Also can we add to the documentation of the Pass the intention behind clearing it?

Good call, and will do.

[Mousius]

I've just chatted with @ekalda around this and we're not sure why the extra buffer appears, it appears to be an unused intermediary buffer and for the tests I ran this fixes it:

Suggested change

	if param not in buffer_map:
	base_addresses.append(
	util.BaseAddress(
	param.name.replace("-", "_"),
	idx,
	_get_region(buffer_info[param].btype, param, scratch_region_map),
	0,
	)
	)
	idx += 1
	continue

I can tidy this loop up afterwards 😸

[Lunderberg]

That works for me locally, on to the CI! Thank you on the ethos-u debugging, and it helps quite a bit.

[Mousius]

Thanks for making the API that little bit more consistent @Lunderberg 😸