XDC physical constraints with TCL interpreter. #15

kboronski-ant · 2022-04-21T12:28:23Z

This PR adds handling of physical constraints expressed via an XDC file.

Supported commands:

get_ports
- by single name
- by multiple names (multiarg call)
set_property
- PACKAGE_PIN (for single port)
- IOSTANDARD (for any number of ports)

Description

Adds an embedded TCL interpreter and a C++ abstraction layer over its C interface, which is used to implement XDC constraints reader. The reader creates VprConstraints that reflect the constraints described in XDC and modifies AtomNetlist to change block properties accordingly.

To read an XDC file, use --xdc_file <filename> option.

Related Issue

(will add later)

Motivation and Context

The current constrain system doesn't allow constraining IOs without knowing exactly what blocks are the going to be. It is also specific to VPR, while XDC seems to be a much more universal format.

How Has This Been Tested?

Manually constraining testarch IOs for LUT design from fpga-interchange-tests and checking results in GUI.

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

acomodi

Looks pretty good. I have added some initial comments below.

acomodi · 2022-04-21T12:29:39Z

libs/libarchfpga/src/physical_types.h

@@ -976,7 +976,7 @@ struct t_mode {
 *      input_string: input string verbatim to parse later
 *      output_string: input string output to parse later
 *      annotations: Annotations for delay, power, etc
- *      num_annotations: Total number of annotations
+ *      num_annotations: Total number opb_typef annotations


probably a typo

typo still needs fix

Not my typo, will fix in another commit.

acomodi · 2022-04-21T12:32:06Z

vpr/src/base/SetupVPR.cpp

@@ -97,6 +99,7 @@ void SetupVPR(const t_options* Options,
    FileNameOpts->CmosTechFile = Options->CmosTechFile;
    FileNameOpts->out_file_prefix = Options->out_file_prefix;
    FileNameOpts->read_vpr_constraints_file = Options->read_vpr_constraints_file;
+    FileNameOpts->read_xdc_constraints_file = Options->XDCFile;


I'd avoid having an option that is strictly related to XDC, or any other constraint file type.

I think it may be better to use the already present read_vpr_constraints_file, and maybe rename it to read_constraints_file and, based on the extension of this file, choose the right way to parse it.

Hmm, I'm not sure about it.
Currently we need BOTH SDC and XDC files, because a separate parser for SDC exists. Ideally both XDC and SDC should use the same backend for reading the files. I mean, we could maybe allow passing both of these files with a single flag, but there would be a hard limit of two files of different format, something I've never seen being done with a CLI interface. Alternatively we can allow reading more XDC files and thus make the flag accept any number of paths. Vivado (and F4PGA) allow reading multiple XDCs, so...

The problem here is that we'd need to add one option for each kind of different constraint files that we provide. The constraint that we are considering here though is only a placement constraint kind of file. We do not care about the SDC at the moment, as it is parsed differently and does not concern constraining blocks to specific locations.

Just imagine that we'll add support for PCF and other kinds of constraint files. We'd need to invoke VPR with a different option everytime. Having instead a dynamic way of determining what is the type of constraint (for now based on the extension of the constraint file), allows us to avoid having multiple options that in the end are doing the same thing, e.g. telling VPR that there is a constraint file to parse.

For now I'm leaving it at having separate argument. We don't infer architecture format from file extension, so why should we do that for constraints. Additionally, I made it possible to pass multiple XDC constraint files (the flag is now called --xdc_files).

vpr/src/base/globals.h

vpr/src/base/oo_tcl.cpp

vpr/src/base/oo_tcl.h

vpr/src/base/xdc_constraints.cpp

acomodi · 2022-04-21T12:50:31Z

vpr/src/base/xdc_constraints.cpp

+        /* TODO: this assumes there's only one port, which just happens to be true in case of fpga interchange.
+         * If we want to handle it better, we should return a list of port ids instead.
+         */


I think it is ok assume that there is one and only one port. I'd rather add an assert that verifies that the block_ports returned are actually of size == 1

An assert would be good, but a explaining why it's here and why it's valid should remain imo. The TODO might be a bit misleading tho, because this isn't going to need any work, unless we end up having more ports per block in the future.

Yeah, sounds good, a brief explanation might be good. The fact is that the get_ports, for our use case at the moment, will only refer to a single PAD.

An improvement to this might be to accept some regex as pin name, which will lead to find more than one port and, for instance, assign the same IOSTANDARD. For now, though, we can keep as a single port only.

I think this should be done by the user in TCL. We could replace AtomPortId returned by get_ports with a list of AtomPortIds and make it accept a list of strings instead. This way the user could generate that list using a regexp or whatever else in TCL.

vpr/src/base/xdc_constraints.h

acomodi

I believe we can proceed by preparing a PR to upstream. I think that these changes do not really require the RR graph PR (which is still pending to be reviewed upstream), therefore we may open a PR with these isolated changes, unless something from the RR graph generation is required.

We can have the discussion about the options to read in the XDC, on whether to have multiple options or one option, with the correct call to be made after analyzing the file extension.

Please, resolve all the conversations you think are solved and fix the remaining ones before opening the PR to upstream.

…ations and bind them to selected ports Signed-off-by: Krzysztof Boronski <[email protected]>

…from XDC command implementation Signed-off-by: Krzysztof Boronski <[email protected]>

Signed-off-by: Krzysztof Boronski <[email protected]>

…perty: handle lists of ports. Signed-off-by: Krzysztof Boronski <[email protected]>

Signed-off-by: Krzysztof Boronski <[email protected]>

github-actions bot added libarchfpga libvtrutil VPR labels Apr 21, 2022

acomodi suggested changes Apr 21, 2022

View reviewed changes

github-actions bot added build lang-make labels Apr 22, 2022

acomodi reviewed Apr 26, 2022

View reviewed changes

github-actions bot added the infra label Apr 27, 2022

kboronski-ant force-pushed the kbor/xdc-interchange branch from 0ec095a to 0d13e8a Compare April 27, 2022 12:14

github-actions bot added scripts lang-shell labels Apr 27, 2022

acomodi force-pushed the acom/fpga-interchange-rr-graph+constants branch from e686856 to f2ae4e5 Compare May 2, 2022 07:30

kboronski-ant force-pushed the kbor/xdc-interchange branch from 07d15e2 to 1cec573 Compare May 2, 2022 11:50

github-actions bot added the external_libs label May 2, 2022

kboronski-ant added 16 commits May 2, 2022 07:48

Added xdc_constraints.cpp

604a105

WIP XDC constraints

5a89005

XDC: improve TCL handling, associate physical pin names with grid loc…

2242ce9

…ations and bind them to selected ports Signed-off-by: Krzysztof Boronski <[email protected]>

Refactor XDC handling, add comments, separate wrappers for tcl stuff …

885153d

…from XDC command implementation Signed-off-by: Krzysztof Boronski <[email protected]>

USe atom block names when getting ports

15a9d7e

Signed-off-by: Krzysztof Boronski <[email protected]>

WIP set_propertty IOSTANDARD, a bit of refactoring

2f1d44e

Signed-off-by: Krzysztof Boronski <[email protected]>

Further cleanup/refactor

f28745b

Signed-off-by: Krzysztof Boronski <[email protected]>

Fixed set_property IOSTANDARD no-return

09a9c6a

Signed-off-by: Krzysztof Boronski <[email protected]>

Moved TCL C++ abstraction into libtclcpp

61ed2c2

Signed-off-by: Krzysztof Boronski <[email protected]>

Use vpr_throw for reporting XDC errors

e9013da

Signed-off-by: Krzysztof Boronski <[email protected]>

Remove _argv0 global

0379c1d

Signed-off-by: Krzysztof Boronski <[email protected]>

libtclcpp: TCL List wrapper. get_ports: return list of ports. set_pro…

bb44f69

…perty: handle lists of ports. Signed-off-by: Krzysztof Boronski <[email protected]>

Indented switch/case

eb5d914

Signed-off-by: Krzysztof Boronski <[email protected]>

make format, added trait requirements for some functions in libtclcpp

d2f0b92

Signed-off-by: Krzysztof Boronski <[email protected]>

Added tk-dev dependency

7e3299a

Signed-off-by: Krzysztof Boronski <[email protected]>

throwaway: enable vtr_reg_strong test run

38ce761

Signed-off-by: Krzysztof Boronski <[email protected]>

kboronski-ant added 4 commits May 2, 2022 07:48

Added tk-dev to install_apt_packages.sh

6662ede

Signed-off-by: Krzysztof Boronski <[email protected]>

Allow parsing multiple XDC files

81519eb

Signed-off-by: Krzysztof Boronski <[email protected]>

libtclcpp: Added TclDynList

f3bf2be

Signed-off-by: Krzysztof Boronski <[email protected]>

Cleanup

9bee76d

Signed-off-by: Krzysztof Boronski <[email protected]>

kboronski-ant force-pushed the kbor/xdc-interchange branch from 1cec573 to 9bee76d Compare May 2, 2022 13:01

github-actions bot removed the external_libs label May 2, 2022

kboronski-ant mentioned this pull request May 5, 2022

XDC placement constraints support for interchange verilog-to-routing/vtr-verilog-to-routing#2021

Open

7 tasks

XDC physical constraints with TCL interpreter. #15

Are you sure you want to change the base?

XDC physical constraints with TCL interpreter. #15

Uh oh!

Conversation

kboronski-ant commented Apr 21, 2022 • edited by kiryk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

acomodi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acomodi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kboronski-ant commented Apr 21, 2022 •

edited by kiryk

Loading