Reworked attributes support

dwRchyngqxs commented 1 year ago

Added support for attributes in unary and binary operations Fix standard compliance for moduleItem attributes

Solves #236.

dwRchyngqxs commented 1 year ago

This is broken. I am investigating the bug.

dwRchyngqxs commented 1 year ago

Done. One traversal did not go through MIAttr and that was added even when the attribute list was empty. Now the traversal goes though and MIAttr is not added when there is no attribute.

zachjs commented 1 year ago

Thank you very much for your thoughtful contribution! Overall, I think this would work as-is to fix a couple issues and implement a feature I've been interested in for some time. Before we merge this, I'd like to split it up into smaller PRs, request some changes, add tests, etc. Given you've already done most of the legwork here, I'm happy to to do as much or as little of this remaining work as you'd like.

PR 1: Fix conversion of `always_*` with attributes

Nice catch! The change you made to AlwaysKW.hs looks right to me. Adding test coverage could be as simple as adding some attributes to test/core/always_spin.{s,}v. If possible, the same commit should add to the changelog something like: Fixed unconverted `always_comb`, `always_latch`, and `always_ff` when tagged with attributes.

PR 2: Forbid attributes on generate constructs

The change to the grammar looks good, but I'm not sure I agree with the change to the AST. I don't think the nested MIAttr structure comes with any clear drawbacks, whereas this new version technically allows for MIAttr [] item. Am I missing something here?

Can we fix the grammar without changing the AST for this? I think this would look something like: addMIAttrs attrs item = foldr MIAttr item attrs. We could also add some checks to test/error/ to ensure things like (* foo *) if (1) wire x; are now disallowed. Although this is technically a breaking change, I don't think it warrants inclusion in the changelog.

PR 3: Support attributes on binary and unary operations

Thank you for working on this! I put off adding this feature because I didn't want to go through the hassle of changing the signature of such pervasive AST nodes. I have another idea:

data Expr
...
    | UniOpA UniOp [Attr] Expr
    | BinOpA BinOp [Attr] Expr Expr
...

pattern UniOp :: UniOp -> Expr -> Expr
pattern UniOp op e = UniOpA op [] e

pattern BinOp :: BinOp -> Expr -> Expr -> Expr
pattern BinOp op e1 e2 = BinOpA op [] e1 e2

This means most existing code would not handle the attributes, but would not have to be changed at all, which I think is fine in most cases. Some files would definitely need to handle attributes on expressions, such as Traverse.hs, TypeOf.hs, and UnbasedUnsized.hs, essentially just to pass them through. Ideally, we'd have test coverage for these cases, too. What do you think of this strategy?

dwRchyngqxs commented 1 year ago

Hello, thank you for reviewing this PR. I will use this PR as PR3 and open other PRs for the rest. For a bit of context I am implementing the full Verilog 2005 syntax in Verismith, and I hope to support some of SystemVerilog using sv2v. As I implemented the standard strictly, I'm finding a lot of standard deviation in sv2v (I won't report them all unless you're intrested) and other open source tools.

About the attributes on module items change:

I am also unsatisfied with my work in this PR. My reason for using a list of attributes (different from the ad-hoc one given below) is that using existing data structures is usually better than implementing them in a roundabout way. This time the advantage is almost barely noticeable: the only processing done on attributes is duplication which require mapping as many times as there are attributes instead of one time; that is to say maybe twice instead of once every super blood moon (that is a thing). There are several ways to handle attributes and I'm not sure which one to choose:

The current MIAttr; like MIAttr []. I think the advantage is only boilerplate traversals have to deal with attributes. The drawbacks are: some possible combination of attribute and module items should not be allowed like an attribute on a generate region, and fine control over attributes is harder to implement as one would have to collect them and match the inner node.
Attaching an attribute list to all the module items constructors that can have attributes. The advantage are: no nonstandard AST, and handling attributes is really easy. The drawback is functions have to handle attributes (can be alleviated by defining pattern and constructor functions as you suggested).
Separating the module items sum type into two sum types, one with attributes and one without attributes; the latter includes a constructor pairing the first type with an attribute list (something like MINonGenerate [Attr] NonGenerateItem). The advantages are: no nonstandard AST, and handling attributes is easy. The drawback are: traversals are more complex, and module item have to ba handled in 2 separate functions.
The MIAttr in this PR; its like the first solution but allows for list sharing when splitting declaration lists for instance (like all the solutions except the first one). MIAttr [] can be avoided using NonEmpty if that really bothers you. The second and third solutions would render PR1 irrelevant. What solution would you like to see implemented? (I chose the 3rd one in Verismith.)

About disallowing attributes for generate constructs:

It can be accepted with a warning if that is better. In my opinion each non standard feature is associated with an unclear semantics, so there may not be a "correct" way to handle them when transforming the ast.

About attributes on expressions:

There are the same 4 ways to handle attributes on expressions. (I chose the 2nd one in Verismith for expressions.) When I will change the code I will probably extend attributes to conditional and increment/decrement attributes.

Definitions like you suggested pattern BinOp op l r = BinOpA op _ l r and binOp op = BinOpA op [] can be useful. That said, most of the code I modified using patterns either passed the attributes as-is, or had to merge them. Dropping attributes, even if it is easier, doesn't seem to me like the right solution: What is the point of parsing them only to drop them at the first operation on the AST? Again what solution do you prefer? I don't mind scrapping all the code I've written to implement another solution.

zachjs commented 1 year ago

I will use this PR as PR3 and open other PRs for the rest.

That sounds great.

For a bit of context I am implementing the full Verilog 2005 syntax in Verismith, and I hope to support some of SystemVerilog using sv2v. As I implemented the standard strictly, I'm finding a lot of standard deviation in sv2v (I won't report them all unless you're intrested) and other open source tools.

Thank you! I am definitely interested in receiving reports of non-standard behavior in sv2v. I'd like to support as much synthesizable SystemVerilog as I can manage. Because sv2v is frequently used in tandem with Yosys, sv2v does support, e.g., non-standard trailing commas. I intend to maintain interoperability with Yosys wherever reasonable.

Per the readme, sv2v's AST is quite permissive compared to the standard. Ideally, sv2v would have a strict AST and a separate but narrow IR, but this would be a big rewrite. I made this design tradeoff before I understood how complex sv2v would become.

About the attributes on module items change:

(quote snipped)

I would prefer not to change the definition of ModuleItem/MIAttr for this. I agree that all 3 of your proposed alternatives are better than the current definition, but I'm not sure that sv2v will benefit substantially by adopting these alternatives. I think we get most (or all?) of the user-visible value by fixing the grammar here.

About disallowing attributes for generate constructs:

It can be accepted with a warning if that is better. In my opinion each non standard feature is associated with an unclear semantics, so there may not be a "correct" way to handle them when transforming the ast.

I think your change and interpretation is correct as-is, once this is split out into its own PR. A parse error (not a warning) seems right as it is clearly not allowed by the standard.

About attributes on expressions:

There are the same 4 ways to handle attributes on expressions. (I chose the 2nd one in Verismith for expressions.) When I will change the code I will probably extend attributes to conditional and increment/decrement attributes.

Definitions like you suggested pattern BinOp op l r = BinOpA op _ l r and binOp op = BinOpA op [] can be useful. That said, most of the code I modified using patterns either passed the attributes as-is, or had to merge them. Dropping attributes, even if it is easier, doesn't seem to me like the right solution: What is the point of parsing them only to drop them at the first operation on the AST? Again what solution do you prefer? I don't mind scrapping all the code I've written to implement another solution.

I am proposing a bidirectional pattern (pattern BinOp op l r = BinOpA op [] l r) as opposed to the above. Most of the conversions don't need to care about attributes. They wouldn't handle them special unless we do so explicitly (as you have in many places already), but they won't drop the attributes either. For example, the constant folding logic in ExprUtils.hs really doesn't need to change at all. I don't think there is any harm in sv2v not constant folding an expression that has an attribute. Even if the BinOp alias I've proposed is only used where a conversion generates new expressions (e.g., Foreach.hs), and as pattern throughout ExprUtils.hs, I think the resulting code simplification, reduced diff, etc., would be worthwhile. Does this motivation seem reasonable? Are there cases I am missing? I greatly appreciate your perspective on this.

dwRchyngqxs commented 1 year ago

I followed your suggestions, but I didn't end up using the pattern you described in pattern matching. This last commit also adds attributes for ternary, but not increments as they are converted to AsgnOp which is not allowed to take attributes from what I've read in the standard.

I would like to add tests for all the PR made out of this one, but I don't really know which directory structure you are following for those. If you mind explaining I will add some.

dwRchyngqxs commented 1 year ago

I accidentally closed this PR when rebasing all my PR.

dwRchyngqxs commented 1 year ago

I added a test and a line in the changelog, this PR is ready for review.

zachjs / sv2v