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

Init Transformer #28

Merged
merged 28 commits into from
Dec 12, 2019
Merged

Init Transformer #28

merged 28 commits into from
Dec 12, 2019

Conversation

leonardt
Copy link
Owner

This provides the initial base class for a transformer visitor.

This implements a polymorphic method visit that recursively visits the AST dispatching on the node type.

There is a basic pattern of replacing a node of type T with type T (e.g. replace identifier "x" with identifier "y").

There is a more complex pattern of replacing an expression with an expression (e.g. replace identifier "x" with expression "z - w". In this case, the user implements a visit(std::unique_ptr<Expression> node) method and dispatches on the runtime type, rather than implementing visit(std::unique_ptr<Identifier> node). This is because we cannot provide two implementations of visit(std::unique_ptr<Identifier> node) (one that returns an Expression and one that returns Identifier). This issue arises when recursively visiting nodes that can have Identifier children, but not Expression children. Right now the pattern is safe (albeit more verbose for the case when we have to check the Expression type), in that it doesn't allow substituting an Expression for an Identifier in a child that can only be an Identifier (e.g. a declaration of a wire or reg). So, if the user wants to replace IDs with Expressions, they can only do so inside children where a more general Expression is accepted.

Open to ways on how to make this easier to use (should probably also provide some macros to do the subtype checking), but this functionality will enable inlining of operators in the coreir backend (replace inputs in0/in1 with a symbol/expression).

Also, we make the AST node fields public so that transformers can modify them.

@leonardt
Copy link
Owner Author

Also this currently doesn't implement precedence checking for binary operators, but will at least always insert wrapping "( )" for children that are not simple IDs/Nums

Copy link
Collaborator

@rsetaluri rsetaluri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good impl of visitor pattern!

Only suggestion is to move the transformer code into its own file if its not a hassle.

@leonardt
Copy link
Owner Author

Reorganized into separate header/source, not too familiar with cpp library organization, but does that seem reasonable? (Or should I merge the transformer decl back into a single header for simplicity?)

@rsetaluri
Copy link
Collaborator

Good to have the header and cpp in different files. LGTM

@leonardt leonardt merged commit 8578a85 into master Dec 12, 2019
@leonardt leonardt deleted the transformer branch December 12, 2019 20:58
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 this pull request may close these issues.

2 participants