-
Notifications
You must be signed in to change notification settings - Fork 430
Add Schema ID to RR Graph Formats #3366
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
Conversation
|
Note that if an XML file does not contain this attribute, it can still be read. An error is raised only when the schema_id attribute is present and does not match the schema used by VPR. |
AlexandreSinger
left a comment
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.
Thanks Amin! Some minor comments below.
So, just to confirm, if the XML file does not have this attribute, this code will not error out? My concern is that many users of VTR generate the RR-graph XML using external tools. So is our solution to this is just to tell them not to set this attribute? That way they "opt-out" of this error checking.
Yes, you are right. If this attribute doesn't exist in XML file, we don't error out. |
vaughnbetz
left a comment
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 minor changes
| */ | ||
| inline void set_rr_graph_schema_file_id(unsigned long schema_file_id, void*& /*ctx*/) final { | ||
| // Only check if the schema file ID is not 0. If it is 0, it means capnproto is not enabled. | ||
| // Thus, we cannot check the schema file ID mismatch. |
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 comment moment is incomplete. Don’t you also allow older rr graphs with no schema to come in? Might also want the to explain that we’re checking the file is compatible when can, but alllowing it to be read if we can’t determine its schema
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.
Changed the comment to the following:
// Only check if schema_file_id_ (set when initializing the class) is not 0.
// If it is 0, it means Cap'n Proto is not enabled, so we cannot check for a schema file ID mismatch.
// This function is only called when the RR graph file being read contains a schema file ID.
// If it does not, this function is not called, and the RR graph can be read without performing
// the schema file ID check.
|
Thanks @vaughnbetz and @AlexandreSinger for your review. I’ve addressed your comments. Please let me know if you have any further feedback. |
AlexandreSinger
left a comment
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.
Thanks @amin1377 LGTM!
I am happy that the RR graph writer now has access to the route verbosity as well.
This PR adds a schema file ID to the RR graph attributes. The ID is included in both the XML and Cap’n Proto (binary) formats. When reading an RR graph, VTR now loads the schema ID stored in its internal schema and compares it with the ID found in the XML or binary file. If the IDs do not match, an error is raised.
@AlexandreSinger: I believe this also addresses your earlier concern about including an ID for the XML file.
Below is the first line of the RR graph XML, showing the added schema ID:
For the Cap’n Proto file: each time the schema is generated, Cap’n Proto assigns a random 64-bit number in the format
@0x.... This number is stored as the schema ID in both the XML and binary RR graph files.