Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

General Lifting Transformation#364

Merged
ScottCarda-MS merged 30 commits intomasterfrom
sccarda/GeneralLifting
Mar 16, 2020
Merged

General Lifting Transformation#364
ScottCarda-MS merged 30 commits intomasterfrom
sccarda/GeneralLifting

Conversation

@ScottCarda-MS
Copy link
Contributor

@ScottCarda-MS ScottCarda-MS commented Mar 10, 2020

Separates out the lifting (hoisting) logic of the classical control transformation into its own transformation that can be used by future features.

Adds IsSelfAdjointable to the BuiltIn record so it can be used in the new transformation logic to accurately create references to these built-in operations.

Trailing whitespace was removed in the relevant files. (You may want to hide whitespace changes when reviewing)

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I haven't made it through the entire PR yet. I'll come back to it in a couple of hours.
All in all things are coming along nicely! I think this is starting to look pretty clean!

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I finished looking through it. Overall this is looking really nice, well done!

I added a few more comments. The three most important ones are:

  • potentially supporting lifing in functions
  • having the logic for determining whether a block should be lifted live in one place
    Also, I'd go with a slightly different choice for what to expose and what not. The easiest rather than commenting here is probably for me to make a PR into your branch that you can look at.

Copy link
Contributor Author

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

I would like to hold off on implementing the lifting of functions until we have a use-case for that feature, and a concrete code path to test that feature in. I don't want to create this functionality without the ability to see concretely that it works as intended.

@ScottCarda-MS ScottCarda-MS marked this pull request as ready for review March 14, 2020 00:10
Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I created to clean-up tasks; one for supporting lifting in functions, the other for absorbing the logic for determining whether something can be lifted into LiftContent.

Co-Authored-By: bettinaheim <34236215+bettinaheim@users.noreply.github.com>
@ScottCarda-MS ScottCarda-MS merged commit 571ad78 into master Mar 16, 2020
@bettinaheim bettinaheim deleted the sccarda/GeneralLifting branch March 16, 2020 17:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants