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

Compatibility between UDP default property and UDPDefinition.default_assignment #213

Open
maltaisn opened this issue May 8, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@maltaisn
Copy link

maltaisn commented May 8, 2024

Declaring an UDP like:

property my_udp {
    component = reg;
    type = boolean;
    default = false;
};

And registering the UDP definition:

class MyUDP(UDPDefinition):
    name = "my_udp"
    valid_components = {Reg}
    valid_type = bool
    default_assignment = False

This will fail with error:

error: The property definition for the feature extension 'my_udp' uses a different 'default' definition from what this tool expects.

It seems like the comparison is being made with an AST node for false instead of the value itself.

@amykyta3
Copy link
Member

amykyta3 commented May 8, 2024

Thanks!
Yeah this looks like it might be a bug. I see in my testcases i do not cover the case where default_assignment is a boolean. Will look into it!

@amykyta3 amykyta3 added the bug Something isn't working label May 8, 2024
@amykyta3
Copy link
Member

amykyta3 commented May 8, 2024

In the meantime, double-check whether you meant to use the get_unassigned_default() method instead. That one is subtly different and in my opinion, way more useful than SystemRDL's weird "default assignment" semantics.

  • "default assignment" = what the property gets assigned if the user specifies it without an assignment operator. In your example, my_udp; would imply my_udp = false;
  • "unassigned default" = what the API will return for the property if the user never assigned or specified the property at all.

More analysis on the interpretation of "default assignments" can be found here: https://systemrdl-compiler.readthedocs.io/en/stable/dev_notes/rdl_spec_errata.html#meaning-of-the-user-defined-property-default-attribute

@maltaisn
Copy link
Author

maltaisn commented May 9, 2024

Also it appears that when not specifying the default in the UDP, the default default is systemrdl.rdltypes.NoValue instead of false if I'm interpreting Table 7 in the spec correctly:
image

@amykyta3
Copy link
Member

amykyta3 commented May 9, 2024

Table 7's "default" column isn't really applicable to UDPs. Rather, that column is generally applied to built-in RDL properties. Even then, I have found that some properties have further clauses described that contradict with Table 7.

I have found that the specification creates several contradictions when it comes to UDPs. Some of them, they call out explicitly which is good. For example clause 15.1.1-d:

The default attribute can result in some inconsistencies relative to the behavior of built-in properties
to the language, especially relating to boolean properties. Built-in booleans in SystemRDL are
inherently defaulted to false. With user-defined boolean properties, the default can be specified to
be true or false. A default of true creates an inconsistency with respect to SystemRDL property
assignments.

Regarding an un-specified UDP default implied default assignment, I have made the interpretation that it results in an un-defined value (aka NoValue) based on the example in 15.2.2-Example1:

property a_map_p { type = string; component = addrmap | regfile; };
...
a_map_p; // The property has been bound to the map but it has not been
         // assigned a value so its value is undefined

Given the text in 15.1.1-d and and the above example, the interpretation that is most consistent in my opinion is that if a UDP does not provide a default in its definition, then any implied value assignments will remain undefined, regardless of type.

This is just one of many things that the SystemRDL spec leaves open to interpretation. In the end, I have to do my best to make a fair and sensible interpretation that is least surprising to the user. UDPs are one part of the spec that are pretty sloppily defined so I get a lot of questions about them. I will add it to the growing list of things I have documented here: https://systemrdl-compiler.readthedocs.io/en/stable/dev_notes/rdl_spec_errata.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants