-
Notifications
You must be signed in to change notification settings - Fork 387
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
Initial place matrix serialization/deserialization support. #989
Initial place matrix serialization/deserialization support. #989
Conversation
- Adds library for handling capnp files and generic NdMatrix support. Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
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.
Looks reasonable to me. I like how it is all controlled via the VTR_ENABLE_CAPNP define.
I've got a couple of comments below.
VPR_THROW(VPR_ERROR_ROUTE, "DeltaDelayModel::write unimplemented"); | ||
void read(const std::string& file) override; | ||
void write(const std::string& file) const override; | ||
const vtr::Matrix<float>& delays() const { |
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.
Is it necessary to expose this implementation detail? Part of the motivation for making the PlaceDelayModel interface was to hide details like this.
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 reason was ease of implementation of OverrideDelayModel serialization, see #989 (comment)
I know how to remove this method if needed, but it is really about what to do about #989 (comment) that changes what the "solution" is here.
private: | ||
std::unique_ptr<PlaceDelayModel> base_delay_model_; | ||
std::unique_ptr<DeltaDelayModel> base_delay_model_; |
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.
Is it necessary to change this from the abstract PlaceDelayModel to DeltaDelayModel? The idea was that any (potentially non-delta delay model) could be combined with this class to provide fix-up overrides.
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.
Serialization would require a mirroring of the inner model type. We could create a string registry of types, and then serialize the string. However, I didn't want to take that additional complexity right now given how few models we have.
Do we want to add such a registry now?
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost Thanks for addressing all the comments! |
Description
Bind
--read_placement_delay_lookup
and--write_placement_delay_lookup
to SerDes (implemented using Cap’n Proto).Motivation and Context
See justification in #707
This PR has predecessor PRs:
How Has This Been Tested?
Unit testing on SerDes, and some integration testing with Symbiflow.
Types of changes
Checklist: