-
Notifications
You must be signed in to change notification settings - Fork 404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AP] Added APPack to the AP Flow as a Full Legalizer #2892
[AP] Added APPack to the AP Flow as a Full Legalizer #2892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions.
Also, you should add the new command line options to the VPR command line documentation (.rst file)
vpr_setup_.Segments, | ||
arch_.directs, | ||
flat_placement_info, | ||
false /* is_flat */); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why is_flat is always false? If you're going to move this around as part of the initial placement / detailed placement separation you can ignore this comment, but eventually we should make sure we work with flat routing (which will just change the rr-graph for the placer delay lookup/matrix calculation I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the placer delay model always wants the rr-graph without the intra-cluster routing. Strange we pass the flat in when it must be false, but reasonably harmless.
// TODO: This was taken from vpr_api. Not sure why it is needed. Should be | ||
// made part of the placement and verify placement should check for | ||
// it. | ||
post_place_sync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sure the pin locations on blocks match the pin locations on the tile where they were placed (for equivalent_sites). I agree it probably makes more sense to put it at the end of placement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into how this is being used in the vpr_api and it is used after both try_place and reading a placement from a file. Its not super simple to remove, so I will leave this as a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, don't worry about it then -- we'd just move a call from one spot to two spots, which is arguably worse.
b85149e
to
35cb34d
Compare
7698d72
to
29bf3b7
Compare
APPack is a Full Legalizer which modifies the Packer and Placer to try and improve their solutions based on a given Flat Placement. Integrated this into the AP Flow by converting the Partial Placement generated by the Global Placer into a Flat Placement and calling the Packer and Placer. Kept the original, Naive Full Legalizer as something to compare to and since it may be useful to keep this legalizer around. Added tests for both the Naive and APPack Full Legalizers, leaving the other tests to always run on the default AP Flow.
29bf3b7
to
25e2450
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs look good, thanks.
APPack is a Full Legalizer which modifies the Packer and Placer to try and improve their solutions based on a given Flat Placement. Integrated this into the AP Flow by converting the Partial Placement generated by the Global Placer into a Flat Placement and calling the Packer and Placer.
Kept the original, Naive Full Legalizer as something to compare to and since it may be useful to keep this legalizer around.
Added tests for both the Naive and APPack Full Legalizers, leaving the other tests to always run on the default AP Flow.