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

Implement interface_array & module_array #3200

Merged
merged 2 commits into from
Sep 4, 2022

Conversation

hs-apotell
Copy link
Collaborator

@hs-apotell hs-apotell commented Sep 2, 2022

Implement interface_array & module_array

Dependent on UHDM PR chipsalliance/UHDM#790

@alaindargelas
Copy link
Collaborator

alaindargelas commented Sep 3, 2022

Interface arrays and module arrays do not help anything in the elaborated model, they are like keeping the model folded.
At most, they have their place in the non elaborated model. They have no place in the elaborated model.
In the elaborated model we need the bit blasted view like we bit blast the generate statements loops.

@@ -17362,6 +17427,117 @@ design: (work@bottom1)
\_logic_net: ([email protected]), line:20:25, endln:20:26
|vpiTypedef:
\_logic_typespec: , line:22:8, endln:22:8
|vpiModuleArray:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant information

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interface arrays and module arrays do not help anything in the elaborated model

Of course they do. May not be for simulation but for tools they provide the ranges and the location information. In the elaborated model, both the array and the blasted view is required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For Generate statements we do not keep any folded view, we bit blast, same with the gate arrays, we have to be consistent. You can keep the module_arrays and instance_arrays in the non-elaborated model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Plus tools like Yosys and Verilator would see 2 views of the same thing, errors in perspective.
This PR cannot go in as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I followed your lead on this. #3101 is your change where you introduced interface_arrays in the elaborated model. This PR is following the same convention for module_arrays. Is #3101 wrong?

There is no duplication in the elaborated model. The arrays are pointing to the blasted elements (not recreating them) and the array itself is unique within the tree. Has introduction of interface_arrays in the elaborated tree with #3101 introduced any problems that I am not aware of?

I can make the necessary changes to remove the module_arrays from the elaborated model but I strongly disagree with your assessment on this.

@alaindargelas
Copy link
Collaborator

alaindargelas commented Sep 3, 2022

I can't reconcile the explanation here:
#3198 (comment)

and the actual logs in the PR, the 2 don't match. I see a bunch of new entries that are redundant in the logs you added to the PR.

Example:
UnitElab.log
These entries existed already, why there are new entries with no particular new info:

\_module: work@my_module ([email protected][0][0][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][0][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][1][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][1][1]), file:top.v, line:29:3, endln:29:44

Do they have the same ID as the actual module instance parented to the parent module?

@hs-apotell
Copy link
Collaborator Author

Backtracking a bit

#3101 is a change you introduced. This added interface_array to elaborated tree. interface_array/Interfaces hold references to the blasted instances i.e. interface.

With the above change, I expected InterfArrayBind to also change. But the PR doesn't have any affect on this test. The test file itself includes interface_arrays but not as port entries but as a member of the module. This use case is not covered by your update. That's what is reported as #3198.

Also, your PR #3101 did not add the interface_arrays to the non-elaborated tree. This PR i.e. #3200 adds that too.

This specific PR i.e. #3200 was following your example from #3101 to add module_arrays to elaborated tree and the non-elaborated tree. Since the non-elaborated tree was still missing the interface_array entries, this PR adds that too.

In all of this change, there are no new modules or interface are instantiated. Both interface_array and module_array hold references to the interface and module respectively that are already created and existent in the elaborated tree. For non-elaborated tree, the interface_array/Interfaces and module_array/Modules containers are not populated, as expected since the non-elaborated tree doesn't have the blasted view of these arrays.

Hope this clarifies the confusion.

@hs-apotell
Copy link
Collaborator Author

Do they have the same ID as the actual module instance parented to the parent module?

Yes, they have the same ID. The entries you have shown in your comment are elements of module_array/Modules container. This container holds references to the instantiated modules after blasting the array.

This is happening in UHDMWriter.cpp:L3493-L3510 here in this change.

Comment on lines 17490 to 17537
|vpiModule:
\_module: work@my_module ([email protected][0][0][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][0][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][1][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][1][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][2][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][0][2][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][0][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][0][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][1][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][1][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][2][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][1][2][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][0][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][0][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][1][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][1][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][2][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][2][2][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][0][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][0][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][1][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][1][1]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][2][0]), file:top.v, line:29:3, endln:29:44
|vpiModule:
\_module: work@my_module ([email protected][3][2][1]), file:top.v, line:29:3, endln:29:44
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of these vpiModule entries are not new instances. They are pointers to the existing instances in the elaborated tree. module_array/Modules holds references to all the instantiated module.

@hs-apotell hs-apotell marked this pull request as ready for review September 3, 2022 15:20
@hs-apotell
Copy link
Collaborator Author

@alaindargelas Our discussion was paused without a conclusion. This PR still has the interface_array and module_array in elaborated tree. If you still disagree that this is incorrect, please let me know so I can update it and repost the change. I need to follow up this with other xxx_array for non-elaborated tree which is likely to use #3101 and this PR as a model.

I would highly recommend reviewing your own change in #3101 and this PR closely before making a conclusion. The instances of interface_array and module_array are unique and interface_array/vpiInterface and module_array/vpiModule are not new instances but are references to existing instance so there is no duplication.

@alaindargelas
Copy link
Collaborator

Do they have the same ID as the actual module instance parented to the parent module?

Yes, they have the same ID. The entries you have shown in your comment are elements of module_array/Modules container. This container holds references to the instantiated modules after blasting the array.

This is happening in UHDMWriter.cpp:L3493-L3510 here in this change.

OK, got it now. Thanks for the detailed explanation. I'll merge.

@alaindargelas
Copy link
Collaborator

@hs-apotell update UHDM in this branch, I commented the non-standard extension -> conflict in UHDM

For non-elaborated tree - Add both interface_array & module_array
For elaborated tree - Add module_array (interface_array exist already)
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.

2 participants