Remove atom_net global context mutation from packer

[AmirhosseinPoolad]

While making the packer parallel, we removed these writes to the global context and nothing went wrong. This WIP PR does the same to VTR master. If these mutations are not needed, removing them will make the code cleaner (less "why is it doing this?") and could potentially make packing a bit faster.

The reason I think things should be mostly fine (other than passing the strong tests) is that atom_net lookup data is only ever changed after packing (add_atom_clb_net is only called after packing is done) and atom_clb is the same (set_atom_clb is only called after packing).

[AlexandreSinger]

There's one big issue here, with these changes free_pb no longer cleans up the CLB lookup data. It is not an issue as free_pb is also called only during packing, with one exception*. However, this new free_pb doesn't do what it's name implies and could be confusing and also breaks the exception. I am not sure how to fix this, hence why the PR is WIP.

The intention of free_pb is to literally free the data associated with a given pb. Since there are global lookups between pbs and other pieces of code, this code also cleans up that lookup value.

The reason it was cleaning up the CLB lookup data is actually historic, and an oversite on my part. Before the Cluster Legalizer existed, the original packer literally created clusters on the spot. These clusters were ill-formed (they were missing critical data), which required them to be destroyed at the end of packing and completely regenerated which was confusing. I completely removed all updates to the CLB netlist; I guess the free_pb passed under the radar.

One note, free_pb is used after clustering; but only when a cluster is being destroyed (I think). This is because ownership of the pbs were given to the netlist after packing and its the job of the cluster netlist to clean up the data (note: I think this is not a good idea, but its just how it was implemented). TBH the update of the lookup between CLB and atoms should probably be updated in the CLB netlist itself, not in some random free method. Such as here:

vtr-verilog-to-routing/vpr/src/base/clustered_netlist.cpp

Lines 172 to 179 in 661debe

	void ClusteredNetlist::remove_block_impl(const ClusterBlockId blk_id) {
	//Remove & invalidate pointers
	free_pb(block_pbs_[blk_id], g_vpr_ctx.mutable_atom().mutable_lookup().mutable_atom_pb_bimap());
	delete block_pbs_[blk_id];
	block_pbs_.insert(blk_id, NULL);
	block_types_.insert(blk_id, NULL);
	block_logical_pins_.insert(blk_id, std::vector<ClusterPinId>());
	}

If this is causing no changes to the output (which I believe it is), I completely agree with these updates. To deal with the line you commented out, you should add in the clustered_netlist.cpp file above some code to update the global lookup betwen CLBs and atoms (which is where this should have been anyways).

[AlexandreSinger]

This is not going to work how you would expect. pbs are stored as a hierarchy. The pb found in block_pbs_ is the root pb for the entire cluster. pb_atom will only return the atom_blk_id associated with a leaf pb (i.e. a primitive). Since you are passing in a root pb, this will never return an atom.

Clusters actually contain many atoms, not just one.

I would recommend iterating over the lookup for this cluster and collect all of the atom_blk_ids, then set all of the atoms to an invalid cluster. Or even better, since this is a bimap I think, you may be able to just tell the bi-map to invalidate all atoms which point to a given cluster blk id.

[AmirhosseinPoolad]

Ah yes I forgot that free_pb is recursive. Yeah I think in that case it's worth rethinking this from a code organization stand point, since we'll be basically duplicating the tree traversal logic which I don't really feel good about. I think I'll just remove the atom_net mutation in pack.cpp for this PR and leave the free_pb changes for another PR since it'll need more extensive reworks to do properly.

[AlexandreSinger]

exists*

[AmirhosseinPoolad]

Hey @AlexandreSinger, I removed the free_pb changes and I think this can be merged, can you do another review? Thanks a lot!

[AlexandreSinger]

LGTM. Can you please raise an issue regarding the other code so we can handle it in the future?