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

Infrastructure for AST transformations and factored out delay transformation #1508

Merged
merged 13 commits into from
Dec 20, 2022

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Dec 19, 2022

The main motivation of this PR is to apply the after delay transformation only optionally when needed by the target.

Currently, the after delay transformation is handled in between some other important passes in GeneratorBase and cannot easily be moved such that it can be called directly by our code generators before or after invoking GeneratorBase. Therefore, this PR introduces a Transformation interface which allows the target generators to register transformations which are then automatically applied.

This PR implements the AfterDelayTransformation and factors out the target specific code into a new interface DelayBodyGenerator which the targets need to provide.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I think this looks great! Thanks for doing this! Aside from making the transformation optional, this code cleanup was much overdue. I've left some minor comments.

/**
* Apply the AST transformation to all given reactors.
*/
void applyTransformation(List<Reactor> reactors);
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to take in the entire resource so that the interface is more general.

Copy link
Collaborator Author

@cmnrd cmnrd Dec 20, 2022

Choose a reason for hiding this comment

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

Yes, my first intuition was also to pass the model and/or the resource. However, it appears that there is some intelligence built into generating this list of reactors (this is handled by the instance graph). We would need to duplicate that code or refactor it to make it reusable, but I wanted to avoid spending too much time on this. Also I am not sure which information other than transformations we would need. I suggest to revisit this as part of fixing #1510

Co-authored-by: Marten Lohstroh <marten@berkeley.edu>
@cmnrd cmnrd merged commit 3e871f6 into master Dec 20, 2022
@cmnrd cmnrd deleted the after-transform branch December 20, 2022 07:18
@cmnrd cmnrd added enhancement Enhancement of existing feature cleanup labels Dec 20, 2022
@lhstrh lhstrh added refactoring Code quality enhancement and removed enhancement Enhancement of existing feature labels Jan 26, 2023
@lhstrh lhstrh changed the title Provide an infrastructure for AST ransformations and factor out the after delay transformation Infrastructure for AST ransformations and factored out delay transformation Jan 26, 2023
@petervdonovan petervdonovan changed the title Infrastructure for AST ransformations and factored out delay transformation Infrastructure for AST transformations and factored out delay transformation Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants