Skip to content

Conversation

@WhiteNinjaZ
Copy link
Contributor

Description

This PR fixes issue #2532. Several benchmarks including difeq2.v and mcml.v where failing parmys on the 7-series due to the 7-series DSP blocks having unequal input widths (25x18). This was caused due to parmys assuming that input widths where always equal in its multiplier splitter. This PR removes that assumption and allows for both types of multipliers to be split properly.

How Has This Been Tested?

Several Verilog designs with a variety of multiplier sizes where synthesized onto a modified version of the k6_frac_N10 arch that included multipliers of various sizes and different input widths (i.e. 18x25). The 7series_BRAM_DSP_carry.xml arch was also used for testing.

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

@WhiteNinjaZ WhiteNinjaZ changed the title Generalize Parmys multiplication Generalize Parmys Mult_Split to Allow for Multipliers Whose Input Widths are not Equal Jun 16, 2025
Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @WhiteNinjaZ, great fix! I didn't review your code changes in Parmys (Leaving that for someone more experienced in that part of the codebase) but since you added a new circuit to vtr_nightly_test2/vtr_xilinx_qor you should generate golden results for it because it would fail the NightlyTestManual CI test otherwise.

Keep in mind that I'm changing the way the results for this test is being parsed in #3138 so make sure you rebase/merge your changes after that PR is merged and generate golden results on top of the new master.

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left some minor comments. I agree with Vaughn's comment during the weekly sync up that we should formally verify this somehow. Are there any Parmys tests we can add a small multiplier test to where this code can get hit?

@AlexandreSinger
Copy link
Contributor

We should also run the NightTestsManual on this branch once it is finalized. Changes in Synthesis make me nervous since it can affect almost all testcases. I think this change should be safe, but its good to be careful.

@vaughnbetz
Copy link
Contributor

@WhiteNinjaZ : we should try to land this one ... a few comments above to look at, and one strong failure (maybe a QoR change, I hope)?

@WhiteNinjaZ
Copy link
Contributor Author

@vaughnbetz Sorry for the delayed response. I have been busy with other projects. It appears that the one failing test is coming from routing in Xilinx strong taking too long. It's a bit odd though since when I run the regression test locally on the most recent version of VTR everything is fine.

@vaughnbetz
Copy link
Contributor

Could just be a different placement or routing due to a different round off. It's 15 s so presumably a small circuit so I suggest just updating the golden result; I'm not worried about a cpu time change on one small circuit.

@vaughnbetz
Copy link
Contributor

@AlexandreSinger @AmirhosseinPoolad : can you take a look at the changes and approve if this looks good so we can merge it?

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for resolving my comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants