Skip to content

Conversation

@hs-apotell
Copy link
Collaborator

Model class hierarchy change for select nodes

bit_select, and other select classes use the vpiParent pointer (as refobj) to provide parent and actual_group access. bit_select.vpiParent.vpiParent is the "true" parent of the bit_select and bit_select.vpiParent.actual_group is access to the actual. However, vpiParent edges in the graph are weak references and ignored for all traversals (including VpiListener, UhdmListener, and vpi_visitor). Tail of parent edges are missing in the UHDM output since the nodes are ignored. The traversal mode also generate unexpected results during runtime because of the ignored edges.

Changing the hierarchy so that ref_obj works as a base (intermediate class in the class hierarchy, yet instantiable) and other select classes (including bit_select, indexed_part_select, part_select, and var_select) are subclasses of ref_obj. This gives access to ref_obj.actual_group and ref.vpiParent can be used for the usual parenting purposes.

  • Updated class file generation logic to support generating class files for intermediate nodes in the class hierarchy.

  • Updated vpi_visitor, ExprEval, and clone_tree logic to compensate for the model hierarchy change.

  • Few code improvements in ElaboratorListener and BaseClass

  • Moved UhdmId, VpiFile, and VpiParent to BaseClass since these are required properties for all models in the class hierarchy

@alaindargelas alaindargelas marked this pull request as draft May 23, 2023 22:49
@alaindargelas
Copy link
Collaborator

alaindargelas commented May 23, 2023

Please fix all failures.
The most problematic of all is: https://github.com/chipsalliance/Surelog/actions/runs/5062235106/jobs/9087500482

Your change breaks the Yosys plugin code it looks like.
You'll have to work with @tgorochowik @hzeller to see how this can be introduced without breaking all the client codes.

@hs-apotell hs-apotell force-pushed the select branch 2 times, most recently from 0faf554 to 9b3c7b8 Compare May 24, 2023 09:31
@hs-apotell
Copy link
Collaborator Author

@tgorochowik @hzeller Could either of you assist with the necessary changes to the yosys plugin codebase

@hzeller
Copy link
Collaborator

hzeller commented May 25, 2023

Can you open a feature request on the yosys plugin and describe what changes in the UHDM data structure would happen after this is submitted ? Then the relevant implementation details can be discussed there.

@alaindargelas
Copy link
Collaborator

Also massive diffs need to be updated here

@hs-apotell
Copy link
Collaborator Author

Can you open a feature request on the yosys plugin and describe what changes in the UHDM data structure would happen after this is submitted ? Then the relevant implementation details can be discussed there.

chipsalliance/synlig#1743

hs-apotell added a commit to Apotell/Surelog that referenced this pull request May 29, 2023
hs-apotell added a commit to Apotell/Surelog that referenced this pull request May 29, 2023
hs-apotell added a commit to Apotell/Surelog that referenced this pull request May 29, 2023
hs-apotell added a commit to Apotell/Surelog that referenced this pull request May 29, 2023
hs-apotell added a commit to Apotell/Surelog that referenced this pull request May 29, 2023
hs-apotell added a commit to Apotell/yosys-f4pga-plugins that referenced this pull request May 30, 2023
UHDM model hierarchy changed to enforce vpiParent as weak reference.
@hs-apotell hs-apotell marked this pull request as ready for review May 30, 2023 03:47
@hs-apotell
Copy link
Collaborator Author

@alaindargelas @hzeller This is ready for review and merge. I have open PR chipsalliance/yosys-f4pga-plugins#522 to fix the plugin issue. The non-vendor build will continue to fail until UHDM PR is merged and tag updated.

Also, as a proof of concept I have #3681 with this change and the change in plugin repo as a patch. The plugin build succeeded.

@hs-apotell hs-apotell force-pushed the select branch 2 times, most recently from f22cf2e to b8ed6d0 Compare May 30, 2023 13:56
@wsipak wsipak mentioned this pull request Jun 2, 2023
@mglb
Copy link

mglb commented Jun 2, 2023

Input:

module top(output int o);
   typedef struct packed {
      logic [9:0] min_v;
   } filter_ctl_t;

   filter_ctl_t [1:0][2:0] a;
   assign a[0][0].min_v = '1;
endmodule

UHDM diff between master and the PR:

--- surelog-a85ff7c1dcf411ff1b7b2b48b5dad17224885197/uhdm.txt   2023-06-02 13:39:12.983968769 +0200
+++ surelog-b8ed6d0a4485f39762c96e60036036a34282efdf/uhdm.txt   2023-06-02 13:39:12.991968881 +0200
@@ -465,24 +465,24 @@
       |vpiParent:
       \_cont_assign: , line:7:11, endln:7:29
       |vpiName:a[0][0].min_v
       |vpiActual:
-      \_bit_select: (a), line:7:11, endln:7:12
+      \_bit_select: ([email protected][0][0].min_v.a), line:7:11, endln:7:12
         |vpiParent:
-        \_ref_obj: ([email protected][0])
-          |vpiParent:
-          \_hier_path: (a[0][0].min_v), line:7:11, endln:7:12
-          |vpiName:a[0]
-          |vpiFullName:[email protected][0]
+        \_hier_path: (a[0][0].min_v), line:7:11, endln:7:12
         |vpiName:a
+        |vpiFullName:[email protected][0][0].min_v.a
         |vpiIndex:
         \_constant: , line:7:13, endln:7:14
           |vpiDecompile:0
           |vpiSize:64
           |UINT:0
           |vpiConstType:9
       |vpiActual:
-      \_bit_select: , line:7:13, endln:7:14
+      \_bit_select: ([email protected][0][0].min_v), line:7:13, endln:7:14
+        |vpiParent:
+        \_hier_path: (a[0][0].min_v), line:7:11, endln:7:12
+        |vpiFullName:[email protected][0][0].min_v
         |vpiIndex:
         \_constant: , line:7:16, endln:7:17
           |vpiDecompile:0
           |vpiSize:64
@@ -586,19 +586,17 @@
       |vpiParent:
       \_cont_assign: , line:7:11, endln:7:29
       |vpiName:a[0][0].min_v
       |vpiActual:
-      \_bit_select: (a), line:7:11, endln:7:12
+      \_bit_select: (a[0][0].min_v.a), line:7:11, endln:7:12
         |vpiParent:
-        \_ref_obj: (a[0])
-          |vpiParent:
-          \_hier_path: (a[0][0].min_v), line:7:11, endln:7:12
-          |vpiName:a[0]
+        \_hier_path: (a[0][0].min_v), line:7:11, endln:7:12
         |vpiName:a
+        |vpiFullName:a[0][0].min_v.a
         |vpiIndex:
         \_constant: , line:7:13, endln:7:14
           |vpiParent:
-          \_bit_select: (a), line:7:11, endln:7:12
+          \_bit_select: (a[0][0].min_v.a), line:7:11, endln:7:12
           |vpiDecompile:0
           |vpiSize:64
           |UINT:0
           |vpiConstType:9

I guess a[0][0].min_v.a visible in the tree generated from the PR is a bug?

On a side note, both trees seem to be invalid, see: #3691

hs-apotell added a commit to Apotell/yosys-systemverilog that referenced this pull request Jun 9, 2023
hs-apotell added a commit to Apotell/yosys-f4pga-plugins that referenced this pull request Jun 9, 2023
UHDM model hierarchy changed to enforce vpiParent as weak reference.
hs-apotell added a commit to Apotell/yosys-systemverilog that referenced this pull request Jun 9, 2023
bit_select, and other select classes use the vpiParent pointer (as refobj) to
provide parent and actual_group access. bit_select.vpiParent.vpiParent is the
"true" parent of the bit_select and bit_select.vpiParent.actual_group is
access to the actual. However, vpiParent edges in the graph are weak
references and ignored for all traversals (including VpiListener,
UhdmListener, and vpi_visitor). Tail of parent edges are missing in the UHDM
output since the nodes are ignored. The traversal mode also generate
unexpected results during runtime because of the ignored edges.

Changing the hierarchy so that ref_obj works as a base (intermediate class in
the class hierarchy, yet instantiable) and other select classes (including
bit_select, indexed_part_select, part_select, and var_select) are subclasses
of ref_obj. This gives access to ref_obj.actual_group and ref.vpiParent can be
used for the usual parenting purposes.
@alaindargelas alaindargelas merged commit 99e5da8 into chipsalliance:master Jun 11, 2023
@hs-apotell hs-apotell deleted the select branch June 11, 2023 05:52
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