Skip to content
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

libs.tech: Fix klayout extraction of diodes #225

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

smunaut
Copy link
Contributor

@smunaut smunaut commented Oct 17, 2024

Diodes were extracted at the wrong hierarchy level. Matthias (KLayout author) was kind enough to debug this issue and came up with this patch.

See related discussion :

https://www.klayout.de/forum/discussion/2605/device-extracted-at-wrong-level-of-hierarchy#latest

But as a summary, KLayout's booleans use the first argument's hierarchy as reference. so it's important to make sure to use a "cell local" layer/polygon as first argument to ensure extraction at the proper level

Diodes were extracted at the wrong hierarchy level.
Matthias (KLayout author) was kind enough to debug this issue
and came up with this patch.

See related discussion :

https://www.klayout.de/forum/discussion/2605/device-extracted-at-wrong-level-of-hierarchy#latest

But as a summary, KLayout's booleans use the first argument's
hierarchy as reference. so it's important to make sure to use a
"cell local" layer/polygon as first argument to ensure extraction
at the proper level

Signed-off-by: Sylvain Munaut <[email protected]>
@sergeiandreyev
Copy link
Contributor

Hi @atorkmabrains, @FaragElsayed2, could you please review this PR?

@smunaut
Copy link
Contributor Author

smunaut commented Oct 22, 2024

Can someone re-run the check ? It was just a timeout fetching klayout ...

Copy link
Contributor

@atorkmabrains atorkmabrains left a comment

Choose a reason for hiding this comment

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

@smunaut I'm not sure if that change would fix the issue in general or not. Because it depends on the specific test case provided. If we change the order of the layer and have the "activ" inside the the layer and antenna_d_mk outside and large marker. The hierarchy will be corrupted as well.

Basically, I think the issue is within Klayout engine itself. It should be able to push all devices down the hierarchy based on the netlist provided. As long as the all the device layers exist, It shouldn't matter which layer is top and which is bottom.

cc @KrzysztofHerman @sergeiandreyev


# ==== dpantenna diode ====
dpantenna_p = pactiv.and(antenna_d_mk)
dpantenna_n = nwell_drw.covering(dpantenna_p)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sergeiandreyev @smunaut I don't agree with that change.

@smunaut
Copy link
Contributor Author

smunaut commented Oct 23, 2024

Yes, you can create some weird hierarchy that would break this rule and still be extracted wrong.
It would be very weird to have the recog diode layer drawn on the "outside" cell ...

And ATM the standard cell library, which uses the sane way of drawing diodes is broken and this patch fixes it.
To me it makes a whole lot more sense to place the diode where the "Recog Diode" layer is than where the NWell or PWell are defined which is most likely up the hierarchy.

Could KLayout be better ? Sure, but you guys chose it as your LVS engine and according to its author (which we can reasonable assume knows his stuff), this is the best way to implement the diode LVS extraction ...

@atorkmabrains
Copy link
Contributor

@smunaut Having one of the recognition layers on top of the series of devices happens if you have a large array of devices for example and it's kind of common practice on that side when people draw a massive array of devices.

I'm not arguing about how the layout is done or where the diode should be placed in the hierarchy, that's not part of my concern, but we as rule deck developers, we have to handle all possible scenarios the user might through at us fundamentally, regardless where the layer is placed in the hierarchy.

In LVS, there is no such thing called "Recog Layer" or "hierarchy layer". The LVS engine (Either in Open Source or Commercial Tools) looks for a device "Identification" layer which is usually a derived layer of a combination of multiple layers. In "Commercial Tools", it continues further the analysis and check where it should place the device in the hierarchy to perfectly match the input. Hierarchy manipulation in Klayout needs some updates to make it function better and more reliably. BTW, on a side note, I believe Matthias is a great guy, for me he's a hero, he is doing this all aside his main job which is extremely incredible. We choose Klayout as it was the best "Open Source" LVS tool out there. It's intuitive and much easier to use than Magic.

Matthias has recommended this change based on the unique example that you have provided and most likely he has analyzed the rule deck hierarchy against that change and saw the hierarchy breaks. I believe it can't be generalized to make this pass as it would be case by case.

Now going to the "standard cells" and we should follow, we were not responsible of that part. That's a call @sergeiandreyev and @KrzysztofHerman has to do not me. As far as LVS goes, if it passes LVS regression, I would say it's up to you to decide if you want to incorporate that or not.

And for my comment that I don't agree with the change is using covering command. I prefer to use inside in such scenarios.

I'll try to rerun actions now if it passes.

@atorkmabrains
Copy link
Contributor

@sergeiandreyev @KrzysztofHerman I don't have the permissions to rerun actions, Could you please do that for us?

@KrzysztofHerman
Copy link
Contributor

@atorkmabrains I have just triggered the workflow

@smunaut
Copy link
Contributor Author

smunaut commented Oct 23, 2024

Having one of the recognition layers on top of the series of devices happens if you have a large array of devices for example and it's kind of common practice on that side when people draw a massive array of devices.

Yes. But having that drawn on a different hierarchy level (i.e. in the top cell) rather than in the same cell as the array of device is what I'd find weird. Beside, in that case, It'd be pretty understandable if those were extracted in the hierarchy level where that recognition layer is drawn.

And for my comment that I don't agree with the change is using covering command. I prefer to use inside in such scenarios.

So ... it's doing exactly what you prefer. It's literally removing covering to replace it with an inside

@atorkmabrains
Copy link
Contributor

@KrzysztofHerman The run fails because it's not able to pull klayout for some reason. Not sure why.

@atorkmabrains
Copy link
Contributor

It seems that Matthias has put some form of traffic filter on the website as the same commands pass in all the other test cases but only fails in that one.

@KrzysztofHerman Would it be possible to keep the klayout deb files in a different server and update the location to pull it off from that location?

@KrzysztofHerman
Copy link
Contributor

@atorkmabrains I could store the .deb files on our external machine. Please point me out the exact version of the package.

@sergeiandreyev
Copy link
Contributor

@KrzysztofHerman
Copy link
Contributor

@atorkmabrains @sergeiandreyev I can setup a small http server to host the klayout_0.29.0 Debian package which the could be downloaded during regression by wget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants