[prim] duplicate primitives between OT and Ibex

[sjgitty]

At the moment we have a few duplicate primitives in the Ibex tree and the OpenTitan tree. We should rectify this. We've talked about a shared tree that we can vendor into OT (and Ibex), not sure if we're ready to cross that river just yet. Other options are to keep them in Ibex when they're duplicated and vendor into OT, but that makes the separation a bit happenstance.

[tjaychen]

i think having a separate repo for this level of shared stuff is a good idea! lowrisc_prim or something.

On Wed, Dec 18, 2019 at 3:44 PM Scott Johnson notifications@github.com wrote:

At the moment we have a few duplicate primitives in the Ibex tree and the OpenTitan tree. We should rectify this. We've talked about a shared tree that we can vendor into OT (and Ibex), not sure if we're ready to cross that river just yet. Other options are to keep them in Ibex when they're duplicated and vendor into OT, but that makes the separation a bit happenstance.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/1231?email_source=notifications&email_token=AAH2RSXPBPGM45BK2LQCRGTQZKYVDA5CNFSM4J4VDEB2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IBPRVKA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSQQT7H5GTMIHEJPTOLQZKYVDANCNFSM4J4VDEBQ .

[msfschaffner]

Indeed, I am also in favor of moving them into a separate repo since that makes it clear where the "root" for these files is.

[eunchan]

If primitive cells are disintegrated from the opentitan repository, it may trigger the separation of common DV environment too. Also, I expect it will be a headache to dealing with process-dependent libraries (SRAMs, synchronizer, CG, Glitch-free MUXes, etc)

One thing we could do is we could compile separately between OT and Ibex. It is doable in DC, VCS, Xcelium but not sure Verilator supports multiple libraries. This can avoid any duplicated naming issue but doesn't solve the disengagement the Ibex primitives from OT's.

Another idea is to just separate the primitive files to the new repo and doesn't manage it other than updating the RTL, and keeping any verification/ FPV in OT. This will create extra steps to update the design in OT to not trigger CI failure.

[tjaychen]

that's a good point on all the common DV components. I would think if we separate this into its own repo, we'd want to move the DV along with it so that it can be pretty self contained. This would imply the headaches you described in the first part...where the "top_earlgrey" common dv components would effectively need to be promoted to 'lowrisc' or 'opentitan' common dv components probably...

I can't remember if this is something @sriyerg had thought about already though.

I think the larger point you're bringing up though, is not whether we want a separate repo for primitives, but rather what is our end goal in how we want all of this stuff to be organized, from common IPs, the top_earlgrey type of stitching, to common cells.

I know as part of the top level reorg discussion, I effectively am suggesting we lump the common IPs under a "core-like" entity, but a good argument can be made that they should be separated into individual submodules as well. So perhaps it makes more sense to discuss what we want to end up like first, then work backwards to see how various pieces should be separated.. ie..are we organized around the opentitan delivery, or are we organized around individual components that can be used to stitch something.

[sjgitty]

I would like to hear from @asb and @imphil since they've done some thinking on this concept all the way back to our June meetings.

[imphil]

I'll think about it and discuss it with Alex next week.

[msfschaffner]

Hey guys, I have come across another instance of this duplicate primitives issue, which is due to the recent vendor-in of Ibex and the icache:

https://github.com/lowRISC/opentitan/blob/74adc5c615eaf07aa55dc96c7bbd42e4594b0874/hw/vendor/lowrisc_ibex/ibex_icache.core#L8-L10

The sim_shared dependency includes a couple of prims in the shared folder within the Ibex repo, which leads to prim duplicates when performing a top-level build.

Certain tools like Synopsys DC don't like that and error out when a module is being overwritten during analysis. I am currently trying to find a workaround for this, but the tool won't let me simply waive that error message. Any ideas of how we could change the dependency structure here such that we can get around this issue?

Maybe it would make sense to reconsider the shared primitive repository idea that came up a while ago, since I think this won't be the last time we run into this problem...

[msfschaffner]

CC @tomroberts-lowrisc @GregAC

[GregAC]

The quick and easy fix is to update the vendoring so we don't bring duplicated modules back into OT.

I'll put together a PR for this instance but yes long term we need a better solution such as a separate shared repository where such things can live.

[msfschaffner]

@GregAC, could you find a workaround for this issue?

[msfschaffner]

Is the lowrisc:ibex:sim_shared core file needed for OpenTitan targets? If not, are you ok with removing this manually for now to get Synthesis going again? We are about to branch off into Bronze...

[GregAC]

lowrisc:ibex:sim_shared is needed. It includes various RTL files the ICache needs. It should be refactored into RTL files and sim only files but I don't think this would solve your issue.

I think the conflict is in prim_assert. A local fix would be to remove the lowrisc:prim:assert dependency from sim_shared.core. Potentially we can do this in a patch file for the ibex vendoring @tomroberts-lowrisc any thoughts?

I believe @rswarbrick has been working on some vendor tool updates to make it easier to support these cases where we have duplicated files.

[rswarbrick]

The vendor tool updates are in PR #2077, but I'm not sure they'd make much difference here. I think I agree that the easiest approach is to add a vendoring patch. It's a bit ugly, though. I hope we can make a "shared infrastructure" repo sooner rather than later to break the dependency cycle.

[msfschaffner]

Actually, prim_assert is not the conflicting one. We've had that for a while now, and it kind of worked since the duplicate file "only" redefines macros (that is not to say that this did not cause problems...). The files that break synthesis now are the ones that redefine modules with the same name, e.g. prim_generic_ram_1p...

Hence, I think the RTL/sim corefile refactoring may still work and solve this problem for the time being. However, in order to avoid this in the future, we should really consider moving to a shared infrastructure repo soon...

[imphil]

we should really consider moving to a shared infrastructure repo soon...

It's on my agenda for this quarter, but figuring all necessary parts out will take time.

[tomeroberts]

@msfschaffner Can you try it again now that #2115 has landed? Hopefully nothing is duplicated across now.

[msfschaffner]

Hi @tomroberts-lowrisc, thanks for splitting the dependencies! I ran locally and it looks like this resolved the problem. We should be able to see the dashboard working again as well within 24h: https://reports.opentitan.org/hw/top_earlgrey/syn/latest/results.html

[tjaychen]

should we close this issue now? seems like it's been resolved.

[imphil]

We do now vendor in almost all code properly from OpenTitan into Ibex. Some further cleanups depend on dvsim patches which are stuck in the review queue in OT, and we'll follow up with those cleanups as they get unblocked. (https://github.com/lowRISC/ibex/issues/985 tracks all remaining work on the Ibex side.)