The-OpenROAD-Project-Attic / FastRoute

LEF/DEF-based port of Iowa State's open-source FastRoute 4.1
BSD 3-Clause "New" or "Revised" License
50 stars 22 forks source link

fastroute should gracefully die on bad pin placement #8

Closed tajayi closed 5 years ago

tajayi commented 5 years ago

Priority: Medium

Details: @oharboe stumbled into an issue where the ioPlacer improperly placed thousands of IO pins illegally at the same location. This resulted in a mystic crash in fastroute. Fastroute should be able to detect bad behavior like this and gracefully die

Steps to reproduce Try to route a design with bad pin placement (several pins on the same location). Ideally, this should be added as a unit test that is expected to fail

oharboe commented 5 years ago

The tiny-tests now contains a test-case for lots of pins. Obviously all tests should pass, so in that way we've got this problem tracked.

eder-matheus commented 5 years ago

We've implemented a function that checks if there are two or more IO pins in the same position and layer, and if it happens, the execution is stopped, followed by an error message like this: "ERROR: n pins in position (x, y), layer z"

Are there any other possible issue with pin placement that we should check before execute FastRoute?

tajayi commented 5 years ago

That's the only one we've stumbled into so far. As I suggested, we might want to add a "check design sanity" step right before global route to check for common design flaws.

Thanks

eder-matheus commented 5 years ago

Issue closed. Please, check commit ae0cda29438210e6a744bbfd69fd3d4dc38430fe.

oharboe commented 5 years ago

The exit code is zero when the pin-layout is invalid, this stops higher level scripts from detecting the problem and stopping the flow:

https://github.com/The-OpenROAD-Project/FastRoute4-lefdef/commit/ae0cda29438210e6a744bbfd69fd3d4dc38430fe#r34677057

tajayi commented 5 years ago

agree with @oharboe

oharboe commented 5 years ago

@tajayi Backing the same drum: there needs to be automated pre-submit tests to enforce the required non-functional behaviors like the non-zero exit code when the tool flow should stop.

Nobody reads a 1500 page specification and if you change the specification, there's no way to enforce it.

A specification must be encoded as a series of automated pre-submit tests.

eder-matheus commented 5 years ago

Please, check commit 2373bb12a157ab9bf9669c06c5e3d9ecc0563225 to see the non-zero exit code fix.

oharboe commented 5 years ago

Can we have unit tests for this? Maybe boost unit tests?

tir. 13. aug. 2019, 22:22 skrev Eder Monteiro notifications@github.com:

Please, check commit 2373bb1 https://github.com/The-OpenROAD-Project/FastRoute4-lefdef/commit/2373bb12a157ab9bf9669c06c5e3d9ecc0563225 to see the non-zero exit code fix.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/The-OpenROAD-Project/FastRoute4-lefdef/issues/8?email_source=notifications&email_token=AAVLJZSHORRYKCWXQA2Y5UTQEMJZPA5CNFSM4IK2O2ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4G33XY#issuecomment-520994271, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVLJZTF33VZIPTCO62LXB3QEMJZPANCNFSM4IK2O2ZA .

eder-matheus commented 5 years ago

We have other issues pending with a short deadline, but we'll try to implement unit tests after fix those issues.

oharboe commented 5 years ago

gotit. my message to the powers that be would be that this is a marathon and that the best time to set aside time for quality is at the beginning. quality is like tying your shoelaces: you run faster and safer.

ons. 14. aug. 2019, 05:10 skrev Eder Monteiro notifications@github.com:

We have other issues pending with a short deadline, but we'll try to implement unit tests after fix those issues.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/The-OpenROAD-Project/FastRoute4-lefdef/issues/8?email_source=notifications&email_token=AAVLJZTLHLPEPEKYOWOZ4ZDQENZTNA5CNFSM4IK2O2ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4HSELI#issuecomment-521085485, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVLJZVW4Y6NMARGW7V7YM3QENZTNANCNFSM4IK2O2ZA .

eder-matheus commented 5 years ago

Since @tajayi's request was implemented, I'm closing this issue. We'll work in unit tests in the near future.