[AP] Full Legalizer

[AlexandreSinger]

The Full Legalizer in the AP flow takes a partial placement (of any legality) and returns a fully legal placement. A legal placement is a set of legal clusterers of atoms and legal places for those clusters to be placed.

To fully test the AP flow, made a temporary Global Placer which will just place the APBlocks at the center of the device. This will be fixed later.

Also includes basic tests to ensure the AP flow does not regress.

[AlexandreSinger]

@vaughnbetz Do you know what this error means? This is showing up in my ch_intrinsics basic test for the AP flow when the highest level of assertions are on.

The error is coming from an ASSERT debug message here (after routing when evaluating the slack):

vtr-verilog-to-routing/vpr/src/timing/slack_evaluation.cpp

Lines 286 to 288 in 3e53a93

	VTR_ASSERT_DEBUG_MSG(verify_max_req_worst_slack(timing_graph, analyzer), "Calculated max required times and worst slacks should match those computed from scratch");

Which is defined here:

vtr-verilog-to-routing/vpr/src/timing/slack_evaluation.cpp

Lines 437 to 468 in 3e53a93

	bool SetupSlackCrit::verify_max_req_worst_slack(const tatum::TimingGraph& timing_graph, const tatum::SetupTimingAnalyzer& analyzer) {
	auto calc_max_req = max_req_;
	auto calc_max_req_node = max_req_node_;
	auto calc_worst_slack = worst_slack_;
	auto calc_worst_slack_node = worst_slack_node_;
	recompute_max_req_and_worst_slack(timing_graph, analyzer);
	if (calc_max_req != max_req_) {
	VPR_ERROR(VPR_ERROR_TIMING,
	"Calculated max required times does not match value calculated from scratch");
	return false;
	}
	if (calc_max_req_node != max_req_node_) {
	VPR_ERROR(VPR_ERROR_TIMING,
	"Calculated max required nodes does not match value calculated from scratch");
	return false;
	}
	if (calc_worst_slack != worst_slack_) {
	VPR_ERROR(VPR_ERROR_TIMING,
	"Calculated worst slack does not match value calculated from scratch");
	return false;
	}
	if (calc_worst_slack_node != worst_slack_node_) {
	VPR_ERROR(VPR_ERROR_TIMING,
	"Calculated worst slack nodes does not match value calculated from scratch");
	return false;
	}
	return true;
	}

My theory is that the clustering from this test is so bad that its causing weird errors to pop up. I am thinking I can just remove this testcase and add it back once the Global Placer is fully fleshed out.

[vaughnbetz]

Out of date comment?

[AlexandreSinger]

I just moved this comment; but I think it is correct?

[vaughnbetz]

I think it will age better if you make a "constraints_dir= ..." line and refer to it (as we do for the circuits etc.)

[vaughnbetz]

Probably want to clip to the device grid to avoid a seg fault.
May want to round to nearest instead of using floor () (left / bottom bias?)

[vaughnbetz]

This would also make a good helper function.

[vaughnbetz]

Could be another helper function.

[vaughnbetz]

I'd make this a helper function

[vaughnbetz]

Yes, should call a legality checker right away.

[vaughnbetz]

The error comes from:

vtr-verilog-to-routing/vpr/src/timing/slack_evaluation.cpp

Line 463 in 3e53a93

"Calculated worst slack nodes does not match value calculated from scratch");

It appears to be complaining that the incremental timing analysis & full timing analysis do not agree on the worst case slack. To use incremental timing analysis you have to mark the edges of the timing graph / connections in the netlist whose delay has changed as "dirty". If you didn't do that and moved something around, you can't call incremental timing analysis (but can call the full recompute from scratch).

[AlexandreSinger]

@vaughnbetz The issue I am running into with this slack evaluation is that I have not even touched the timing graph... Whats worst is that this error comes up on the second routing attempt (since it is doing a min channel width sweep). It looks like this error is actually in routing and that my packing & placement just reveals the issue. I think there may be a small issue with how the incremental timing is being performed in the router. The reason this may not have been caught is that this is a debug check and there are only 4 small testcases which have this check turned on (vtr_reg_basic with timing on). I am not too sure how to proceed on this issue beyond turning off this test for now and raising an issue. My guess is something in the router is not marking a change in the routing graph as dirty, but I am not sure where to look!

[vaughnbetz]

OK, that sounds fine. You can turn off the test for now and raise an issue. Please tag Fahri on it ... he's the most likely one to figure it out. Maybe your clustering is so different you show a mistake that usually doesn't show up, as you suggest. Not sure how else you could impact the router. (The timing engine does make callbacks to figure out delays, so if you left out some clustering info that would be a possibility -- maybe it is doing invalid look ups? But it could definitely also be a latent bug that you've exposed).

[AlexandreSinger]

@vaughnbetz Sounds good. I will raise an issue on it. I do not know why I did not think of this before, but I generated the .net and .place files from the AP flow and then fed those into VPR independently of the AP flow and was able to replicate the issue. This shows that it has nothing to do with the AP flow and this is just a really hidden bug that is exposed by my really crappy clustering and placement.

I'll include all of these generated files and the commands to replicate this in my issue.

[AlexandreSinger]

To show how hidden this bug is (and how unlucky I was). This command has the issue:

vpr fixed_k6_frac_N8_22nm.xml ch_intrinsics --circuit_file ch_intrinsics.pre-vpr.blif --net_file ch_intrinsics.net --place_file ch_intrinsics.place --route --device unnamed_device --read_vpr_constraints ../../../../ch_intrinsics_fixed_io.xml --min_route_chan_width_hint 40

And this command does not:

vpr fixed_k6_frac_N8_22nm.xml ch_intrinsics --circuit_file ch_intrinsics.pre-vpr.blif --net_file ch_intrinsics.net --place_file ch_intrinsics.place --route --device unnamed_device --read_vpr_constraints ../../../../ch_intrinsics_fixed_io.xml

Just removing the min channel width hint causes the routing problem to change slightly and remove this issue!

[vaughnbetz]

Weird. I don't suppose there's any chance we forgot to re-timing analyze from scratch after restoring the routing or anything like that (which might be more directly related to the hint)?

[AlexandreSinger]

@vaughnbetz I have resolved most of your comments:

• I have move count models into the Cluster Legalizer (this just made sense)
• I have split the full legalizer into two methods (Clustering and Cluster Placement)
• Added a method to get the containing physical tile loc from the partial placement position of an APBlock
• Added some code to print the .place file after the AP flow (I just forgot to do that)
• I turned off that failing testcase. Will raise an issue soon.

I have purposfully left large parts of the full legalizer un-outlined since I plan to return to it very soon to further clean it up and completely restructure it. I just wanted to push something that is working asap. For example, the verification function from the initial placer was seg faulting due to some data structures the placer used not being initialized (but I do not need the structures at all). My goal is the properly unify my cluster placement with the initial placer.

Regarding the "constraint_dir=" comment, I completely agree with you; but I am avoiding modifying the scripting infrastructure while the CI is down. I also want to do this in its own PR.

Regarding which tile to place the given APBlock, I disagree that it should be the nearest. At the Full Legalizer level, the APBlocks will be hovering over the tile they want to be in (even if it is at position 0.9,0.9 its still in the 0th tile). The issue with VTR is that we uniquely identify tiles by their top-left corner which is super confusing. In reality, I imagine each APBlock by being placed at the center of the tile. My approach does break-down when the tiles are not 1x1; however, this is the job of the partial legalizer to resolve (at least in the current design). Eventually I would like to make the cluster placer try to place cluster blocks to the tile whose center is closest to the centroid location of all blocks in the cluster; but this is a bit down the road. Right now, I went for simplicity. The partial placement is assumed to be valid (verify passed, which ensures all blocks are on the device) and all blocks want to be in whatever tile they are hovering above.

[AlexandreSinger]

Issue has been raised: issue #2754

[vaughnbetz]

Ok thanks for the suggestion summary. Isn't the anchor point of blocks their bottom left corner though?

[AlexandreSinger]

Sorry, yes. My mind is in graphics land, where I consider (0,0) to be the top left corner. The anchor-points of all tiles is the floor of all positions in it. So (0.5, 0.5) is the tile identified by (0,0). TBH mathematically it does not matter if the blocks are placed at the center or corner of tiles, since it would just add a constant offset to all blocks which will have no affect on the HPWL calculation, but it just makes more logical sense; since, as you pointed out, it moves an AP block from (0.9, 0.9) all the way to (0,0) when (1,1) is technically smaller of a displacement.

Edit: My mindset is that APBlocks will snap to the center of each tile. So, using my example, (0.9, 0.9) would rather snap to (0.5, 0.5) rather than (1.5, 1.5); I just have to use (0,0) to identify the tile.

[vaughnbetz]

Thanks ... please merge after you add the comment above and figure out CI failures are spurious etc.