Skip to content

Fix bug in which all non-chain luts are considered length-1 chains. #2363

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KA7E
Copy link
Contributor

@KA7E KA7E commented Aug 8, 2023

Current find_new_root_atom_for_chain assumes that an atom without a driver is the root of a chain, and so all non-chain luts are considered trivial length-1 chains. This fix permits a driverless atom to be a chain root only if it drives another chain atom.

Description

Added a new boolean input, is_non_trivial_chain, to find_new_root_atom_for_chain, initially set to false. If the current block has no drivers, the function can only return it as the new chain root if the boolean is true. The boolean is set to true when the function is called recursively from further down the chain.

Related Issue

Motivation and Context

I noticed this phenomenon when examining lists of pack molecules while debugging a legalizer tool to read in an external flat placement and recreate a legal clustering. My tool packs chains first, and it works better without trivial chains.

How Has This Been Tested?

This has been tested on all Titan23s as part of normal vpr packing and as part of my legalizer tool.

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

Current find_new_root_atom_for_chain assumes that an atom without a driver is the root of a chain. This fix permits a driverless atom to be a chain root only if it drives another chain atom.
@github-actions github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Aug 8, 2023
@vaughnbetz
Copy link
Contributor

Some of the CI test failures may be random changes. E.g.: vtr_nightly_test1_odin:
[Fail]
22:07:36 | k6_frac_2ripple_N8_22nm.xml/fir_pipe_15.v/common min_chan_width_route_time relative value 15.494845360824742 outside of range [0.1,15.0], above absolute threshold 2.0 and not equal to golden value: 2.91
--> this just looks like a minor change on a small design; it's OK to just update the golden results (minimum channel width routing time went up more than 15x, but the overall runtime on this small circuit is still small and minimum channel width route times can have high noise)

This vtr_reg_strong test looks like it actually failed to complete though:
slicem.xml/carry_chain.blif/common vpr_status Task value 'exited with return code 1' does not match golden 'success'
18:55:05 | [Fail]

So that is likely a corner case.

Some other runs seem to have run out of time; relaunching.

@vaughnbetz
Copy link
Contributor

After CI passes (or the errors are found to be spurious as in the small QoR change above), you should also get a QoR comparison on the vtr and titan circuits anyway to ensure nothing bad has happened. I don't expect it would, but it is best to be safe. See https://docs.verilogtorouting.org/en/latest/README.developers/#example-vtr-benchmarks-qor-measurement for a how-to guide.

@KA7E
Copy link
Contributor Author

KA7E commented Oct 16, 2023

Yes, the carry chain test failure is real. The corner case is when the head is the first node encountered in a chain. I have an idea for a fix, stay tuned.

KA7E added 2 commits October 18, 2023 20:04
… for its chain; since it has no driver, check to see if it drives a pattern connection
@KA7E
Copy link
Contributor Author

KA7E commented Oct 19, 2023

It appears that vtr_reg_nightly_test2 and vtr_reg_nightly_test2_odin failed with the message:
Error: The operation was canceled.

All others passed. Re-running just those two.

@KA7E
Copy link
Contributor Author

KA7E commented Oct 24, 2023

These two tests keep stopping with the note "the operation was cancelled" and I am not sure why. Vaughn can you take a look?

@vaughnbetz
Copy link
Contributor

Update: @KA7E is deciding if this is a bug or just a somewhat unexpected way to consider all LUTs part of (length 1 at least) carry chains.

@vaughnbetz
Copy link
Contributor

@KA7E tells me her preference is to not merge (at least for now). It seems like an odd behaviour, but it no longer impacts the legalizer. Since there are CI failures (hopefully spurious) and QoR would have to be checked this would need some work to merge. Putting on hold for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants