[SV] Generic SVAttributes

[uenoku]

Currently SVAttributes are intentionally limited to sv.reg, sv.wire(https://github.com/llvm/circt/commit/761b6107e78f8ef89576754731ff5367a1b9060f) and sv.assign(https://github.com/llvm/circt/commit/3a13fbf0530d66fc1048a7b5ec901725e951241d), e.g:

%bar = sv.reg svattrs [#sv.attribute<"foo">]
sv.assign %bar, 0 svattrs [#sv.attribute<"baz">]
==>
(* foo *)
wire bar;
(* baz *)
bar = 0;

However SV spec says SVAttributes can be attached to everywhere including module/port definitions, statements and expressions (even operators). Actually I have use cases to add vendor specific pragmas to expressions so I want to make SVAttributes more generic. To do this, it would be also necessary to stop inherenting sv.attribute in each op definition. Implementation wise, we can pick and extend the commit(512ae544fbc1a14f) in the original PR.

However there are quite a few problems to extend it to expressions.

1. Transformation must respect sv attributes SVAttributes might not be semantically discardable(?) so we should change transformations (canonicalizers, HWCleanUp, ..). For example, propagating attributes (e.g. add(a, add(b, c)){sv.attributes="foo"} => add(a, b, c){sv.attributes="foo"}) is in general illegal. However it is hard to prevent discarding attributes attached to dead sub operations (e.g. add(a, add(b, c){sv.attributes="foo"}) => add(a, b, c)).
2. Positions where SV attributes are emitted SV Spec doesn't define where these attributes must be emitted. Hence, expected positions of vendor specific pragams are very random. For example, how to specify the position of emitted attributes for mux(a, b, c):{sv.attribute="bar"}? e.g)

a (* bar *) ? b : c
a ? (* bar *) b : c
a ? b (* bar *)  : c
a ? b  :(* bar *)  c  
a ? b  : c  (* bar *) 

[mikeurbach]

Since the ODM is open for tomorrow, maybe this is something we should discuss. I know that is a tough time for you, but we could potentially touch on it and then loop back to this issue.

[teqdruid]

Do you have a specific use-case?

On Tue, Jun 28, 2022 at 2:11 PM Mike Urbach @.***> wrote:

Since the ODM is open for tomorrow, maybe this is something we should discuss. I know that is a tough time for you, but we could potentially touch on it and then loop back to this issue.

— Reply to this email directly, view it on GitHub https://github.com/llvm/circt/issues/3430#issuecomment-1169247138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALNXYAWTEHKYB6Z4X4KVBLVRNS6JANCNFSM52DJW3HA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[uenoku]

Since the ODM is open for tomorrow, maybe this is something we should discuss

That sounds good!

Do you have a specific use-case?

Yes, our users want to add pragmas to multibit mux like this:

(* synopsys infer_mux_override *)
assign _GEN_0 = array[sel] (* cadence map_to_mux *); 

IR I expect is:

%0 = sv.wire
%1 = hw.array_get %array, %sel {sv.attributes="cadence map_to_mux"}
sv.assign %0, %1  {sv.attributes="synopsys infer_mux_override"}

[mikeurbach]

One thing I was thinking, in order to avoid the whole "SV attributes on arbitrary expressions" problem, was this specific use-case is pretty specific to multi-bit mux. I haven't thought this through all the way, but maybe another angle is to add a comb.multi_bit_mux, support SV attributes there like on reg/wire/assign, and lower to the appropriate thing in ExportVerilog. That might be a different way to achieve this without opening the "SV attributes on arbitrary expressions" can of worms.

On the other hand, that sort of just kicks the can down the road, so maybe we should be biting the bullet and figuring out the right solution to this now...

Anyway, looking forward to discussing in the ODM.

[uenoku]

I haven't thought this through all the way, but maybe another angle is to add a comb.multi_bit_mux, support SV attributes there like on reg/wire/assign, and lower to the appropriate thing in ExportVerilog.

Yes, I think that approach would work. Probably we don't have to add comb.multi_bit_mux since I think it is sufficient to support SV attribute for hw.array_get.

On the other hand, that sort of just kicks the can down the road, so maybe we should be biting the bullet and figuring out the right solution to this now...

Yeah, I think it is inevitable that we want to annotate pragmas to other operations, so we have to figure out the right solution anyway.

[darthscsi]

The standard is explicit where attributes go for on each operation and declaration. Search for attribute_instance in the syntax in the standard.

[teqdruid]

@uenoku Is this done?

[uenoku]

Yes and No. Most operations are not currently supported in ExportVerilog. It should be fairly easy to implement them now.

Supported:

• wire, reg declarations
• assignments
• array indexing

Unsupported:

• [ ] Modules
• [ ] Statements
• [ ] Unary operations
• [ ] Binary operations
• [ ] Ternary operations
• [ ] Variadic operations

[teqdruid]

Yep. We're also probably gonna need to specify a SV attribute for FSM synthesis from behavioral SV.

~John

On Tue, Jun 28, 2022 at 2:34 PM Hideto Ueno @.***> wrote:

Since the ODM is open for tomorrow, maybe this is something we should discuss

That sounds good!

Do you have a specific use-case?

Yes, our users wants to add pragmas to multibit mux like this:

( synopsys infer_mux_override )assign _GEN_0 = array[sel] ( cadence map_to_mux );

IR I expect is:

%0 = sv.wire%1 = hw.array_get %array, %sel {sv.attributes="cadence map_to_mux*}sv.assign %0, %1 {sv.attributes="synopsys infer_mux_override"}

— Reply to this email directly, view it on GitHub https://github.com/llvm/circt/issues/3430#issuecomment-1169269244, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALNXYDQ5MTIWM4WTX6YLJ3VRNVWLANCNFSM52DJW3HA . You are receiving this because you commented.Message ID: @.***>