Skip to content

add pin counting filter option to pack_mol_in_existing_cluster #2585

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

Merged

Conversation

KA7E
Copy link
Contributor

@KA7E KA7E commented Jun 5, 2024

Added enable_pin_feasibility_filter parameter to pack_mol_in_existing_cluster

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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

@KA7E KA7E marked this pull request as ready for review June 5, 2024 01:07
@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jun 5, 2024
@AlexandreSinger
Copy link
Contributor

image

Thats not a good error to see. It looks like our self-hosted machines are not having a great time. One issue for this @KA7E is that the CI is run on EVERY push AND PR raised on a branch. If you are not careful you may be running the CI way more often than you think. This appears to be causing strain on the machines. We may have to wait for the CI to calm down a bit.

I am currently working on getting having the CI get called less and save less information so this is more unlikely to happen. But for now, lets get the other PRs merged in.

@AlexandreSinger
Copy link
Contributor

image

This error pretty clearly has nothing to do with your changes (since this actually ran the CI twice and one of them passed). @vaughnbetz , I wonder if this is caused by the artifacts thing we were talking about earlier this week.

Like this does not look good LOL:
image

But granted I have always seen it over, but usually its only 30 GB not 140 GB. However, reading into it this is actually from CCache not from artifacts:
image

Either way I will look into how we are storing caches and think about how we can reduce it, since it looks like we are storing 400 MB of data per cache which feels like a lot.

Regarding the artifacts thing though, this is either a fluke; or we are having issues with how much artifacts we can store. @vaughnbetz maybe we can follow the steps to reduce the storage from 90 days to 45 days.

@KA7E
Copy link
Contributor Author

KA7E commented Jun 7, 2024

@AlexandreSinger and @vaughnbetz is there a way for this to progress despite the CI issues? This is less crucial to the legalizer than the other two, but still important.

@AlexandreSinger
Copy link
Contributor

@KA7E I have merged in master, which contains my fix to the CI. I wanted to use this branch as a guinea pig, since every time you push it actually used to run the CI twice. Now with these changes it only runs the CI once. Mission success lol.

The CI is much calmer this morning (especially after I killed a bunch of duplicate runs), so hopefully this should pass without issue. The CI should become much more stable as more people merge in my recent PR.

KA7E added 2 commits June 8, 2024 16:52
…ption_to_recluster_util' into add_enable_pin_counting_filter_option_to_recluster_util
@vaughnbetz
Copy link
Contributor

Looks good. For safety, I'd like to get at least one design (e.g. neuron) with a CPU time number before and after the various related PR changes to confirm CPU time in the default flow is unaffected.
That could be done with a QoR sweep but a simpler test that would also work would just be to run one design on a machine with controlled load (e.g. your machine) and post the pack & place & total run times, along with post-route cpd and wirelength. I don't expect any changes, but it pays to be paranoid as it is very, very hard to find a degradation that slips into the master branch.

@vaughnbetz vaughnbetz merged commit 8791d19 into master Jun 11, 2024
53 checks passed
@vaughnbetz vaughnbetz deleted the add_enable_pin_counting_filter_option_to_recluster_util branch June 11, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants