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

[firtool] firrtl.AttributeAnnotation on arrays is ignored by firtool #7934

Closed
kammoh opened this issue Dec 2, 2024 · 3 comments · Fixed by #7948
Closed

[firtool] firrtl.AttributeAnnotation on arrays is ignored by firtool #7934

kammoh opened this issue Dec 2, 2024 · 3 comments · Fixed by #7948

Comments

@kammoh
Copy link

kammoh commented Dec 2, 2024

Following MWE is based on sv-attr.fir test:

FIRRTL version 4.0.0
circuit Foo: %[[{"class": "firrtl.AttributeAnnotation",
                 "description": "keep_hierarchy = \"true\"",
                 "target": "~Foo|Foo"},
                {"class": "firrtl.AttributeAnnotation",
                 "description": "mark_debug = \"yes\"",
                 "target": "~Foo|Foo>w"},
                {"class": "firrtl.transforms.DontTouchAnnotation",
                 "target": "~Foo|Foo>w"},
                {"class": "firrtl.AttributeAnnotation",
                 "description": "mark_debug = \"yes\"",
                 "target": "~Foo|Foo>n"},
                {"class": "firrtl.AttributeAnnotation",
                 "description": "mark_debug = \"yes\"",
                 "target": "~Foo|Foo>n[0]"},
                {"class": "firrtl.AttributeAnnotation",
                 "description": "mark_debug = \"yes\"",
                 "target": "~Foo|Foo>n[1]"},
                {"class": "firrtl.transforms.DontTouchAnnotation",
                 "target": "~Foo|Foo>n"},
                {"class": "firrtl.AttributeAnnotation",
                 "description": "keep = \"true\"",
                 "target": "~Foo|Foo>r"}]]
  ; CHECK: (* keep_hierarchy = "true" *)
  ; CHECK-NEXT: module Foo
  public module Foo:
    input a1: UInt<1>
    input a2: UInt<1>
    input clock: Clock
    output b1: UInt<1>
    output b2: UInt<1>
    ; CHECK: (* mark_debug = "yes" *)
    ; CHECK-NEXT: wire w
    wire w: UInt<1>[2]
    ; CHECK:      (* mark_debug = "yes" *)
    ; CHECK-NEXT: wire n
    node n = w;
    ; CHECK:      (* keep = "true" *)
    ; CHECK-NEXT: reg r
    reg r: UInt<1>, clock
    connect w[0], a1
    connect w[1], a2
    connect b1, xor(n[0], n[1])
    connect r, a1
    connect b2, r

Generated SystemVerilog:

(* keep_hierarchy = "true" *)
module Foo(
  input  a1,
         a2,
         clock,
  output b1,
         b2
);

  wire w_0 = a1;
  wire w_1 = a2;
  wire n_0 = w_0;
  wire n_1 = w_1;
  (* keep = "true" *)
  reg  r;
  always @(posedge clock)
    r <= a1;
  assign b1 = n_0 ^ n_1;
  assign b2 = r;
endmodule

The issue exists in all firtool versions I tried, including 1.95.1 release and the main branch.

@kammoh
Copy link
Author

kammoh commented Dec 2, 2024

The annotation is applied correctly when the --preserve-aggregate=vec (or all, or 1d-vec for this example) is used, but I really don't want to keep all vectors.
Seems that the SVAttributeAttr is removed in LowerTypesPass.

Would it make sense to have LowerTypesPass reapply these attributes to the lowered types?

@uenoku
Copy link
Member

uenoku commented Dec 4, 2024

Hi thank you for the report! #7948 should resolve this issue.

uenoku added a commit that referenced this issue Dec 5, 2024
This PR changes to clone discardable attributes when operation is cloned. 

Fix #7934. AttributeAnnotation adds `sv.attributes` in LowerAnnotation as a discardable attribute which previously LowerTypes dropped.
@kammoh
Copy link
Author

kammoh commented Dec 5, 2024

Awesome 🎉🎉🎉
Thank you for the quick fix!

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 a pull request may close this issue.

2 participants