[rtl/core] split dmem/imem entities and architectures to separated files

[umarcor]

In this PR, the entities and architectures of dmem and imem are split to separated files.

Although most of the codebase in this project is platform/vendor independent, those two components need to be explicitly changed depending on the task. On the one hand, some boards/devices do require using primitives. On the other hand, in order to run architecture tests, a variant optimised for simulation is used.

From a behavioural/logic point of view, this PR should change nothing. The units are split and all the build/run scripts are adapted in order to include/import the new files. In fact, this is a subset of #101. However, no alternative descriptions are added here, and the VHDL sources of subdir setups are kept untouched.

This is also related to #150. Currently, in order to run the architecture tests we are copying the whole rtl subdir, and we replace a single file. That is suboptimal. After this PR is merged, we should be able to adapt the scripts for using the sources in the root and just changing the architecture file.

On a bigger picture, this is related to #110 and edaa-org.github.io. One of the purpose of EDAA is to handle projects with use cases such as this one, where multiple common HDL sources are used but some specific need to be changed depending on the target.

[stnolting]

I really like this concept as I think it would things a lot easier or at least more straightforward. 👍

I will check out the changes on my system. I will also try to update documentation (data sheet and user guide) accordingly.

[umarcor]

I will check out the changes on my system. I will also try to update documentation (data sheet and user guide) accordingly.

Please, take your time, and let me know if I can help. I tried to be careful with not breaking the Quartus, Radiant, etc. but I could not try them.

[stnolting]

Quartus and Radiant setups work like a charm (can't test Xilinx - license issues :sad:).

One question: we replace the default *MEM architecture file by an optimized platform-specific one (like the ice40up SPRAM). Is it VHDL-valid if this replacement file has an entity definition in it? So we have several times the same entity definitions: rtl/core/neorv32_*mem.entity.vhd, (in the package) and in the actual replacement file. GHDL and Radiant seem to be ok with that.

[umarcor]

One question: we replace the default *MEM architecture file by an optimized platform-specific one (like the ice40up SPRAM). Is it VHDL-valid if this replacement file has an entity definition in it? So we have several times the same entity definitions: rtl/core/neorv32_*mem.entity.vhd, (in the package) and in the actual replacement file. GHDL and Radiant seem to be ok with that.

Architecture files should contain the architecture only. Even if some tools might handle exactly the same entity to be defined in multiple places, that is not desirable. Hence, if users want to provide both in a single file, they should remove the entity only and the default architecture files.

However, the point here is that the entities are always the same, that is the interface that makes multiple implementations compatible. Therefore, the idiomatic solution is not to repeat the entity declarations.

[stnolting]

This is how I understand the "split" idea:

• always include the entity files from rtl/core
• include the architecture file of choice (from rtl/core/mem or from anywhere else

If this is correct, then we should remove the entity declarations from setups/osflow/devices/ice40 and from the Radiant example setup, right? This would be the straight-forward approach I think.

On the other hand, it might be confusing for some users (like me 😄) to have VHDL files without an entity. As long as the entity declarations are completely identical everywhere, there should be no problem to keep them in all the memory files, right?

Another thing that came to my mind: We could rework the rtl files to use "entity instantiation" instead of "module instantiation":

    neorv32_int_imem_inst: entity neorv32.neorv32_imem
    generic map (
      IMEM_BASE    => imem_base_c,          -- memory base address
   ...

By this we would no longer need the entity-only files nor the entity definitions in the package file, right?

[umarcor]

This is how I understand the "split" idea:

• always include the entity files from rtl/core

• include the architecture file of choice (from rtl/core/mem or from anywhere else

If this is correct, then we should remove the entity declarations from setups/osflow/devices/ice40 and from the Radiant example setup, right? This would be the straight-forward approach I think.

Correct. That was an oversight on my side. Shall I push it here or will you do it?

On the other hand, it might be confusing for some users (like me 😄) to have VHDL files without an entity. As long as the entity declarations are completely identical everywhere, there should be no problem to keep them in all the memory files, right?

In fact, this is common practice, not something I made up. When there is a single entity and a single architecture, typically both are written in the same file. When there are more than one architecture, sometimes they are written in the same file, sometimes they are split as done here. Hence, many VHDL users are already familiar with this setup.

The weird thing in this case is that multiple different architectures exist, but they all have the same name. As a result, if a user unwantedly adds multiple architectures to the list of files, some tools will "silently" ignore all except the last one. Nevertheless, that is not something to be solved in this PR.

The point with entity declarations being completely identical everywhere is that it holds only until it is not true anymore. That will happen sooner or later. Conversely, if a single declaration of the entity exists, such problems cannot occur.

Another thing that came to my mind: We could rework the rtl files to use "entity instantiation" instead of "module instantiation":

    neorv32_int_imem_inst: entity neorv32.neorv32_imem
    generic map (
      IMEM_BASE    => imem_base_c,          -- memory base address
   ...

That is not exactly the same topic. Yes, given the current setup, we can use a direct instantiation instead of declaring a component.

When using a direct entity instantiation, the architecture needs to be hardcoded (or the analysis order applies). When using components, configurations can be used. Nvertheless, for now, I think it's ok to use entity instantiations, since we are not really using different architecture names. See also https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues/235.

By this we would no longer need the entity-only files nor the entity definitions in the package file, right?

We would need an entity definition, but not the component in the package file.

[stnolting]

Correct. That was an oversight on my side. Shall I push it here or will you do it?

Can you do that? 🙃

In fact, this is common practice, not something I made up. When there is a single entity and a single architecture, typically both are written in the same file. When there are more than one architecture, sometimes they are written in the same file, sometimes they are split as done here. Hence, many VHDL users are already familiar with this setup.

That's right. A single file with several architectures is nothing special. I was just thinking about file with entity-only or architecture-only declarations. But I think that is no big deal.

The weird thing in this case is that multiple different architectures exist, but they all have the same name. As a result, if a user unwantedly adds multiple architectures to the list of files, some tools will "silently" ignore all except the last one. Nevertheless, that is not something to be solved in this PR.

We should have an assert in those multiple-architecture designs to inform the user which architecture is actually used 🤔

When using a direct entity instantiation, the architecture needs to be hardcoded (or the analysis order applies). When using components, configurations can be used. Nvertheless, for now, I think it's ok to use entity instantiations, since we are not really using different architecture names. See also https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues/235.

Maybe we can do this in another PR.

We would need an entity definition, but not the component in the package file.

Still not clear to me. We do have an entity description at the point where we instantiate. Do we still need the entity-only file in rtl/core if we keep entity and architecture together in the rtl/core/mem/* and platform-specific files?

[umarcor]

Correct. That was an oversight on my side. Shall I push it here or will you do it?

Can you do that? 🙃

I wil 😄

The weird thing in this case is that multiple different architectures exist, but they all have the same name. As a result, if a user unwantedly adds multiple architectures to the list of files, some tools will "silently" ignore all except the last one. Nevertheless, that is not something to be solved in this PR.

We should have an assert in those multiple-architecture designs to inform the user which architecture is actually used 🤔

It's technically not possible. Since all the architectures have the same name and belong to the same library, the latest one analysed is the one used (or the tool might fail). In order to have multiple architectures analysed without errors, each of them should have a different name. Thus, that is the case of using "case generate" or "configurations", as discussed in https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues/235.

When using a direct entity instantiation, the architecture needs to be hardcoded (or the analysis order applies). When using components, configurations can be used. Nvertheless, for now, I think it's ok to use entity instantiations, since we are not really using different architecture names. See also https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues/235.

Maybe we can do this in another PR.

👍🏼

We would need an entity definition, but not the component in the package file.

Still not clear to me. We do have an entity description at the point where we instantiate.

What we have at the point where we instantiate is an entity instantiation, no the declaration. We need an entity declaration. You can put that entity declaration in the same file as the instantiation, but that would be strange because it only makes sense if an entity is to be used in a single place. The idiomatic procedure is to have the entity in a separated file.

Do we still need the entity-only file in rtl/core if we keep entity and architecture together in the rtl/core/mem/* and platform-specific files?

If we keep the entity in the rtl/core/mem/*.default.vhd files and also in the platform specific files, then this PR does not make sense at all. That is exactly the current state of the repo and what we are trying to change.

[stnolting]

I wil 😄

Thanks 😄

---

Ok ok, maybe this is a misunderstanding on my side... 😅

With the modifications from this PR we still need to

1. not include the file from rtl/core/men
2. do include a different architecture for the memories (like SPRAM-based)

if we want to use platform-specific memory components instead of the default ones from mem, right?

Thus, we only have one architecture file in our project for - let's say - the IMEM. The architecture is defined via the included file, so in this architecture we could have an assert telling something like "using ice40 SPRAM thingy", correct?

If we use entity instantiation (no component declaration in the package, no entity-only file) the synthesis tool will check for this entity name in its pre-compiled blocks (I see compile order issues there...). There will be only one one file (including entity and architecture) because that is what was manually assigned to the VHDL sources (example: not the default one from rtl/core/mem but a platform specific one from setups/osflow/devices/ice40).

[umarcor]

With the modifications from this PR we still need to

1. not include the file from rtl/core/men
2. do include a different architecture for the memories (like SPRAM-based)

if we want to use platform-specific memory components instead of the default ones from mem, right?

Thus, we only have one architecture file in our project for - let's say - the IMEM. The architecture is defined via the included file, so in this architecture we could have an assert telling something like "using ice40 SPRAM thingy", correct?

That is correct. For any design, users need to include rtl/core plus their architectures (which can be the defaults in rtl/mem, some of the others in this repo, or any other implementation they provide). We can add a report for printing some string that allows the user to identify which one is being used, in case they unwantedly add multiple architectures. However, I don't think it can be an assertion (what do we assert?). That's probably the root of the misunderstanding.

If we use entity instantiation (no component declaration in the package, no entity-only file) the synthesis tool will check for this entity name in its pre-compiled blocks (I see compile order issues there...). There will be only one one file (including entity and architecture) because that is what was manually assigned to the VHDL sources (example: not the default one from rtl/core/mem but a platform specific one from setups/osflow/devices/ice40).

As said, if there is only one file (including entity and architecture), then this PR is nonsense. That'd reduce the PR to:

mv rtl/core/neorv32_?mem.vhd rtl/core/mem

It is correct that, due to all the variants of an architecture having the same name, and because the users are forced to add a single file, the synthesis/simulation would work. However, that is unidiomatic. The language supports multiple architectures per entity, and we are reimplementing that functionality in bash (with lost capabilities because there might be silent mismatches). The idiomatic solution after this PR would be to rename the architectures so that each has a unique name.

[umarcor]

Correct. That was an oversight on my side. Shall I push it here or will you do it?

It's done now.

[stnolting]

It's done now.

Thank you! 😉

That is correct. For any design, users need to include rtl/core plus their architectures (which can be the defaults in rtl/mem, some of the others in this repo, or any other implementation they provide). We can add a report for printing some string that allows the user to identify which one is being used, in case they unwantedly add multiple architectures. However, I don't think it can be an assertion (what do we assert?). That's probably the root of the misunderstanding.

Right, this is a misunderstanding caused by me. I was thinking about something like this:

assert false report "bla bla bla" severity note;

As said, if there is only one file (including entity and architecture), then this PR is nonsense. That'd reduce the PR to:

I think it is a good idea to split the memory files into "default platform-agnostic" and "device specific". But I do not quite understand why we need an entity-only file when we actually add a specific memory VHDL file anyway.

So the default setups will use all files from rtl/core plus rtl/core/mem. A setup with platform-specific memories will use rtl/core and memories from somewhere/else. All VHDL files that implement IMEM/DMEM will have the same entity and entity name (the "prototype" entity is specified in the package file). Sorry for being so pedantic but I just do not understand the point of the entity-only file yet 🙈 😅

[umarcor]

Right, this is a misunderstanding caused by me. I was thinking about something like this:

assert false report "bla bla bla" severity note;

Gotcha. I think that assert false is not necessary.

I think it is a good idea to split the memory files into "default platform-agnostic" and "device specific".

That is the current state of the repo, without this PR.

But I do not quite understand why we need an entity-only file when we actually add a specific memory VHDL file anyway.

We need to define the entity once only, not necessarily in a file alone. The problem with the current approach is that the same entity is defined 5 times at least. All those 5 definitions should be exactly the same, however, that is not the case. As you can see in the changes of this PR, there are at least three different declarations (some have no default base address, some have 0x0, some have 0x80...). You can find that mismatch and fix it manually now; but similar bugs will be introduced each time someone tweaks one of the definitions unaware of all the others. By removing the duplication, it is guaranteed that all architectures correspond to the same exact entity.

All VHDL files that implement IMEM/DMEM will have the same entity and entity name

This requires manually comparing all the duplicates. In practice, it is likely that the entity is not exactly the same, despite having the same name and maybe being compatible with the instantiation syntax. The point of this PR is to remove all the bugs that might arise, by avoiding the existence of mismatching definitions.

[stnolting]

Gotcha. I think that assert false is not necessary.

Oh, really?? Great Scott, I'm a real VHDL beginner sometimes 😅

We need to define the entity once only, not necessarily in a file alone. The problem with the current approach is that the same entity is defined 5 times at least. All those 5 definitions should be exactly the same, however, that is not the case. As you can see in the changes of this PR, there are at least three different declarations (some have no default base address, some have 0x0, some have 0x80...). You can find that mismatch and fix it manually now; but similar bugs will be introduced each time someone tweaks one of the definitions unaware of all the others. By removing the duplication, it is guaranteed that all architectures correspond to the same exact entity.

Now I understand the problem 😄 and I agree. Thanks for clearing!

[stnolting]

I think this is ready to be merged. But let's have a look at the modified documentation again - I mean I had problems to identify the issues this PR is solving, so maybe some of that is still somewhere in the updated docs.. 😉

[umarcor]

Gotcha. I think that assert false is not necessary.

Oh, really?? Great Scott, I'm a real VHDL beginner sometimes 😅

Oh, not at all, mate! That's a "mistake" I do myself so frequently 😆. Although I did cleanup, it's still found in ghdl-cosim https://github.com/ghdl/ghdl-cosim/search?q=assert+false

I think this is ready to be merged. But let's have a look at the modified documentation again - I mean I had problems to identify the issues this PR is solving, so maybe some of that is still somewhere in the updated docs.. 😉

I will reread and reply here explicitly.

[stnolting]

Btw, what about the boot ROM (rtl/core/neorv32_boot_rom.vhd)? 🤔
Should we also split that (maybe in a follow-up PR)? I mean, it is "just" a ROM but it seems like there are some situations, where this cannot be mapped to bitstream-initialized block RAM (#140).

[umarcor]

Btw, what about the boot ROM (rtl/core/neorv32_boot_rom.vhd)? 🤔
Should we also split that (maybe in a follow-up PR)? I mean, it is "just" a ROM but it seems like there are some situations, where this cannot be mapped to bitstream-initialized block RAM (#140).

Yes. In the future, this same approach might be applied to different components. Not only the Boot ROM, but maybe some DSP/ALU, a built-in SPI vs an VHDL version, etc. However, I would not like to put that complexity into the codebase until we actually need it.

The motivation for doing it with the IMEM and DMEM now is twofold:

• We do already need it, because as explained we do have duplicates already.
• In the context of pyEDAA.ProjectModel, handling use cases such as this is an absolute priority. VUnit, OSVVM, UVVM, etc. are not necessarily designed for having "swappable" components. Yet, it is a must in any RealWorld project targeting multiple boards/devices.

Hence, my proposed strategy is to first merge this, then focus on pydoit and/or pyEDAA.ProjectModel (which are very tightly related) in order to have a cleaner solution for handling it. If done correctly, then extending the logic to the Boot ROM or any other component should be straightforward. Nevertheless, I'm good with doing it before, if you prefer that.

[stnolting]

Yes. In the future, this same approach might be applied to different components. Not only the Boot ROM, but maybe some DSP/ALU, a built-in SPI vs an VHDL version, etc. However, I would not like to put that complexity into the codebase until we actually need it.

Right, good point(s).

• We do already need it, because as explained we do have duplicates already.
• In the context of pyEDAA.ProjectModel, handling use cases such as this is an absolute priority. VUnit, OSVVM, UVVM, etc. are not necessarily designed for having "swappable" components. Yet, it is a must in any RealWorld project targeting multiple boards/devices.

Convinced! 😉

[stnolting]

Ready to merge from my side 👍

[umarcor]

I did a not exhaustive read of the datasheet and UG, and I didn't find any missing fix/modification. Hence, I think this is good to merge. We can always enhance the docs afterwards, should we find mismatches.

[stnolting]

Thank you so much for your work!
🚀