systemrdl-compiler icon indicating copy to clipboard operation
systemrdl-compiler copied to clipboard

Questionable interpretation of UDP's "default" keyword behavior

Open j-klier opened this issue 5 years ago • 7 comments

The syntax for property assignment is:

property_name [= expression];

The spec Section 5.1.3.1 says the value is presumed true when the expression is not specified .

When expression is not specified, it is presumed the property_name is of type boolean and the default value is set to true.

The example in #45 demonstrates the issue:

property bool_prop {type = boolean; component=field; default=false;};
addrmap test {
    reg {
        field {
            bool_prop;
        } FIELD[4] = 0;
    } TEST @ 0x0;
};

would return 0 when doing a print(node.get_property("bool_prop")) on the Python side (and 1 when the default is set to true.

These should always be equivalent irrespective of the property declaration.

bool_prop;
bool_prop=true;

j-klier avatar May 29 '20 00:05 j-klier

The behavior of a user defined property's default setting is often misinterpreted and this is actually the intended behavior. See my reply in #30 for a detailed explanation.

amykyta3 avatar May 29 '20 01:05 amykyta3

Isn't the example in 15.2.2 just another bug in an example like these? https://systemrdl-compiler.readthedocs.io/en/latest/dev_notes/rdl_spec_errata.html

This comment in #30 is not backed up anywhere other than the 15.2.2 example.

For normal built-in properties, a no-value assignment implies the property was assigned true. For UDPs, it implies the assignment of its default.

The spec that says the opposite in 15.2. User Defined Properties (UDP) have the same assignment rules as built in properties.

User-defined properties may be assigned like general properties (see 5.1.3).

What's the point of specifying a default value?

15.2.1-c) User-defined properties can be bound to a component without setting a value. 15.2) A user-defined property is bound when it is instantiated within a component definition or assigned a value.

This property does not have a value and must be instantiated within a component in order to be bound. property some_bool_p { type = boolean; component = field; }; You can bind this UDP to all field components by assigning a value. default some_bool_p; // implies some_bool_p=true; This property has an assignment and is bound to all field components with the value of false. property some_bool_p { type = boolean; component = field; default = false; };

We can also use 15.1.1-d to clarify the intended behavior.

A default of true creates an inconsistency with respect to SystemRDL property assignments.

This property binds some_bool_p=true to all field components in the current scope (since it has been assigned a value). property some_bool_p { type = boolean; component = field; default = true; };

When we instantiate the property within a component, it does not change the value. The default assignment was true and the implied assignment is also true. field { some_bool_p; } field1; // Attach some_bool_p to the field with a value of true;

This is inconsistent with a built-in property because the statement above would have changed the value from false to true.

In systemrdl-compiler we have an inconsistency inconsistency when the default value is false. property some_bool_p { type = boolean; component = field; default = false; }; field { some_bool_p; } field1; // Attach some_bool_p to the field with a value of false; A built-in property would have assigned the value of true.

I think the systemrdl-compiler implementation is based on a bug in a comment and does not reflect the actual rules in the spec.

j-klier avatar May 29 '20 15:05 j-klier

@j-klier Re-opening this, since I missed your detailed reply after I initially had closed this.

In principle, I totally agree with you on what the default keyword ought to do. I just want to be extra sure I'm not missing something in the spec's interpretation before I proceed to change the compiler's behavior.

You have a good point regarding your comment on clause 15.1.1-d. That pretty clearly confirms the intended behavior. Worth copying the full text of that clause for posterity:

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.

One last thing that's nagging me, is the text in the opening paragraph of Section 15:

Unlike built-in properties, user-defined properties are not automatically bound to every instance of the specified component’s type; instead they need to be bound per instance and/or component definition.

The above statement is what originally led me to my first interpretation, which I agree, is probably incorrect. That said, what on earth could that mean - that UDPs need to be "bound"? Any ideas on how to interpret that?

amykyta3 avatar Aug 27 '20 04:08 amykyta3

Aagh! Now I'm not totally convinced. Looking at the full 15.2.2-Example1 again, there is another snippet that is interesting:

property some_num_p { type = number; default = 0x10; component = field | reg | regfile };
// ...
field { some_bool_p = true; some_num_p; } field2;

This raises several questions:

  • What is the value of some_num_p in this case? Surely it cannot be True since that's not an integer. (casting it to 1 could be the case ... I guess?).
  • Or does it get "bound" the default value of 0x10? This is consistent with my original interpretation and current implementation. It also aligns with the rest of the comments in that example.
  • What was the author's intent of showing this example in the first place? Given the full context, it appears to be demonstrating the behavior of assignments w.r.t. defaults.

I disagree that this example is a mere typo. It feels a bit too ... intentional. However I do agree that the wording of clause 15.1.1-d subtly contradicts with 15.2.2-Example1.

It seems there are two concepts at play here:

The value assigned if an expression was not given when specifying a property

property some_num_p { type = number; default = 0x10; component = field | reg | regfile };
property some_bool_p { type = boolean; component = field; default = false; };

field {
    ispresent; // = true implied
    rclr; // = true implied
    some_num_p; // = 0x10 implied?
    some_bool_p; // = false implied?
}a;
  • Built-in properties:
    • Very clearly states in 5.1.3.1 that the value is true
    • Also the phrasing uses the word "default" to imply it is referring to this concept specifically:

      When expression is not specified, it is presumed the property_name is of type boolean and the default value is set to true.

  • UDPs:
    • 15.2.2-Example1 as a whole seems to only make sense if the value assigned is the default from the UDP's declaration.

The "assumed" value of a property if not assigned at all

property some_num_p { type = number; default = 0x10; component = field | reg | regfile };
property some_bool_p { type = boolean; component = field; default = false; };

field {
    // ispresent = true assumed due to 5.3.1-b
    // rclr = false assumed
    // some_num_p = ??
    // some_bool_p = ??
}a;
  • Built-in properties:
    • Boolean built-ins are generally assumed false if unassigned (in most cases)
    • Other built-in properties vary wildly and often depend on context
  • UDPs:
    • Ambiguous!
    • Does this also assume the UDP's default value? This would be consistent with 15.1.1-d.
    • Or does it remain "undefined" since it was not "bound" as the intro paragraph in Section 15 states?

amykyta3 avatar Aug 27 '20 05:08 amykyta3

Thanks for highlighting the wording in 5.1.3.1.

When expression is not specified, it is presumed the property_name is of type boolean and the default value is set to true.

I agree that 15.2.2-Example1 is too intentional to be a mistake and the correct interpretation is that the implied expression assigns the default value from the UDP declaration (if available). When the default value is not present, the value should come from the data type according to 5.1.3.4.d. The difference between UDP and pre-defined properties is that pre-defined properties are presumed to have the default = true whereas UDP use the value from the declaration (if any).

Implied expressions

I've added a_map_p to your example to show the difference between a UDP with and without a default value.

property a_map_p { type = string; component = addrmap | regfile | field; };
property some_num_p { type = number; default = 0x10; component = field | reg | regfile };
property some_bool_p { type = boolean; component = field; default = false; };

field {
    ispresent; // = presumed true 
    rclr; // = presumed true
    a_map_p; // = "" implied from default value for string type
    some_num_p; // = 0x10 implied from default value
    some_bool_p; // = false implied from default value
}a;

When is a UDP bound to an object?

A UDP requires an instance or assignment to bind it to the component.

15.2) A user-defined property is bound when it is instantiated within a component definition or assigned a value.

Assignments are outlined in 5.1.3.4. and specifically:

5.1.3.4.c) default property assignment (see 5.1.3.2) 5.1.3.4.d) SystemRDL default value for property type (see Table 7)

Where the type refers to the data type, not the the default value from the property declaration.
The default value from the declaration is not the same as a default property assignment statement. This was covered in https://github.com/SystemRDL/systemrdl-compiler/issues/30, but I think the default property assignment statement is a valid way to bind a property to a component.

property a_map_p { type = string; component = addrmap | regfile | field; };
property some_num_p { type = number; default = 0x10; component = field | reg | regfile };
property some_bool_p { type = boolean; component = field; default = false; };

default some_num_p = 0x20;
field {
    // ispresent = true assumed due to 5.3.1-b
    // rclr = false assumed
    // a_map_p = has not been bound
    // some_num_p = 0x20 from default property assignment
    // some_bool_p = has not been bound
}a;

Summary

My original assumptions were incorrect

  • default value during UDP declaration is different than default property assignment.
  • SystemRDL treats pre-defined properties differently from UDP during implied expressions because it "presumes" pre-defined properties were declared boolean and true.
  • The user must know how the UDP was declared when using implied expressions.

2 new questions/issues were raised:

  • A UDP without a default value should be derived from the data type (see 5.1.3.4.d and Table 7)
  • A UDP default property assignment should bind the property to all matching components in the same scope.

j-klier avatar Sep 03 '20 17:09 j-klier

Is this issue also related to the default value of the UDP never gets assigned to the instances? In this example, I would expect the "mymap" version property to be "1.0" but I get None.

property version {
    type = string;
    component = addrmap;
    default = "1.0";
};

addrmap mymap {
    reg {
        field {} a1;
    } a;
};

RasmusGOlsen avatar Sep 08 '20 07:09 RasmusGOlsen

You have given the property a default value, but no default assignment. I believe you need an assignment statement like this:

property version {
    type = string;
    component = addrmap;
    default = "1.0";
};
default version; // = "1.0" is implied from default value
addrmap mymap {
    reg {
        field {} a1;
    } a;
};

Can you confirm if adding the default assignment statement fixes the issue?

j-klier avatar Sep 09 '20 12:09 j-klier