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

[FEATURE] Access to all node predecessors. #257

Closed
jeras opened this issue Mar 6, 2025 · 3 comments
Closed

[FEATURE] Access to all node predecessors. #257

jeras opened this issue Mar 6, 2025 · 3 comments
Labels
wontfix This will not be worked on

Comments

@jeras
Copy link

jeras commented Mar 6, 2025

I would like an attribute/method for the Node class, that would return an array of the node's parents up to the top node.

The use case is a combination of listing all nodes which are instances of a given global_type_name, with complications related to arrays.

First I create a dictionary with all global_type_name as keys and an array of nodes of this type as values:
https://github.com/jeras/PeakRDL-jinja/blob/devel_jinja_with_SystemRDL/src/peakrdl_jinja/templates/relative.md.jinja#L51-L61

As part of the documentation generator I document a single node, the first one in the node array described above.
https://github.com/jeras/PeakRDL-jinja/blob/devel_jinja_with_SystemRDL/src/peakrdl_jinja/templates/relative.md.jinja#L73

Within this node I list all instances with the same global_type_name.

For paths without arrays, I can just print the full node path and the absolute address.
https://github.com/jeras/PeakRDL-jinja/blob/devel_jinja_with_SystemRDL/src/peakrdl_jinja/templates/relative.md.jinja#L94-L96

For paths using arrays the problem is more complex, since arrays can be part of multiple levels of the hierarchy, for example an array of reg inside and array of regfile.
I need access to all predecessor nodes to get information on array dimensions for each hierarchical level.
Current tools for handling arrays are not powerful enough to easily handle arrays in parent (predecessor) nodes.

I have two ideas how to list all instances for arrays.

  1. Just loop over all array elements for all predecessors and list them all with absolute address. The problem is with large arrays, resulting in long lists.
  2. Instead of a unwrapping the array loops, write the path with array index variables and a formula for the absolute address:
    example32.array_regfile[i0].array_reg[i1][i2]
    @ 0x00000000 + 0x0100 * i0 + 0x0020 * i1 + 0x04 * i2
    

One of the two can be chosen depending on the length of each array (less or more than 4 elements for example).

For now I will try to create the predecessors in Jinja. I still have to see how exactly I will do the looping over all the predecessors and over all array dimensions and elements. I will update this post when I get a prototype.

@amykyta3
Copy link
Member

amykyta3 commented Mar 8, 2025

This is pretty trivial to do with the existing API, and is a pretty rare use-case so I am not sure it really belongs as part of the main API.
Here's a pretty simple utility function that accomplishes this:

def get_ancestors(node: Node) -> list[Node]:
    ancestors = []
    current_node = node
    while current_node.parent:
        ancestors.append(current_node.parent)
        current_node = current_node.parent
    return ancestors

@jeras
Copy link
Author

jeras commented Mar 9, 2025

I am working on a PeakRDL tool, which would use Jinja templates to export to different source code or documentation formats. Compared to other exporters, I tried to avoid any data processing in the PeakRDL code and instead do all the processing in Jinja. The reason to avoid processing in PeakRDL is to avoid creating another API, for example a Jinja template context with only dictionaries. Having a custom API for the Jinja context requires additional documentation. Using the SystemRDL-compiler API (already fully documented) inside Jinja templates, avoids the need for developers to document another API and the need for users to learn it.

Still writing a get_ancestors macro in Jinja, is not a great choice, having it in the compiler would help. For now I only find get_ancestors to be useful for creating documentation (not source code), so I am not sure whether it would be better to have it in the compiler, or just code it in Jinja. I will write a proposal when I make more Jinja templates, to see what works better. Also, having ancestors in the top to node order would be more intuitive, this adds another line of code to the jinja template.

There are also smaller issues. For example the RootNode does not have an absolute address, size or the property is_array. The node size can be used to determine the number of bits in the address. When listing node addresses, the parents size determines the number of address bits (or hex characters in a literal). I can handle this by checking whether the parent is the root node, and in this case use the size of the node itself, otherwise the size of the parent. This adds another check to the Jinja template. Again I am not yet sure if it would be worth making a change to the compiler, or just deal with a bit more code in the template.

@amykyta3 amykyta3 added the wontfix This will not be worked on label Mar 9, 2025
@amykyta3
Copy link
Member

amykyta3 commented Mar 9, 2025

Jinja is a great templating tool and I have used it for many projects, but imposing the restriction on yourself to have all the code generation logic inside your Jinja template is not something I would ever recommend. In practice I have found this type of approach scales extremely poorly and will make your templates far more difficult to understand and maintain. Inevitably you'll need to create several helper utilities, beyond just the get_ancestors() utility we discussed. True that these utilities could be implemented purely in Jinja, but Jinja is not a good general purpose programming language.

As to making part of this of the SystemRDL API - I do not see the justification for adding it as it would not be widely beneficial to other users. You have mentioned several requirements that are extremely specific to your application, so such utility functions would be better to be implemented locally to your project. This would give you more flexibility to tailor them to exactly the behavior you need.

@amykyta3 amykyta3 closed this as completed Mar 9, 2025
@amykyta3 amykyta3 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants