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.

Sign up for GitHub

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

Jump to bottom

Remove optimizer context #990

Merged
merged 13 commits into from
Jun 18, 2020
Merged

Remove optimizer context #990

merged 13 commits into from
Jun 18, 2020

Conversation

jdahm
Copy link
Contributor

@jdahm jdahm commented Jun 4, 2020 •
edited
Loading

Technical Description

Removes the OptimizerContext object. OptimizerContext.{cpp,h} became Lowering.{cpp,h}, and the salient parts were named toStencilInstantiation and restoreIIR. The latter is only used in the IIR builder.

Other changes:

  • Passes no longer take a context, PassManager has a default constructor
  • Hardware options were moved from the, now gone, OptimizerContext to Optimizer/Options.inc
  • Test logic is simplified because they do not have to hold an OptimizerContext

Testing

CTests are updated by this PR.

@jdahm
Copy link
Contributor Author

jdahm commented Jun 4, 2020

launch jenkins

@jdahm jdahm requested a review from Stagno June 4, 2020 06:43
@jdahm
Copy link
Contributor Author

jdahm commented Jun 4, 2020

launch jenkins

@jdahm jdahm mentioned this pull request Jun 4, 2020
Closed
@jdahm
Copy link
Contributor Author

jdahm commented Jun 4, 2020

launch jenkins

@jdahm jdahm requested a review from mroethlin June 4, 2020 21:25
@Stagno
Copy link
Contributor

Stagno commented Jun 5, 2020

@jdahm could you provide some guidance on how to review it? If you write a list of things that changed (e.g. where the logic of the OptimizerContext was moved to) we can focus on those points. thx!

@jdahm
Copy link
Contributor Author

jdahm commented Jun 5, 2020 •
edited
Loading

@Stagno no problem. Here it is:

OptimizerContext.{cpp,h} became Lowering.{cpp,h}, and the salient parts were named toStencilInstantiation and restoreIIR. The latter is only used in the IIR builder.

Other changes:

  • Passes no longer take a context, PassManager has a default constructor
  • Hardware options were moved from the, now gone, OptimizerContext to Optimizer/Options.inc
  • Test logic is simplified because they do not have to hold an OptimizerContext

@jdahm
Copy link
Contributor Author

jdahm commented Jun 9, 2020

@Stagno @mroethlin Let me know if you would like to walk through the code together. I'd like to get this merged soon, as most other PRs will conflict with it. Once it's merged we can go ahead and split the projects (finally!)

Stagno
Stagno suggested changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@Stagno Stagno left a comment

Choose a reason for hiding this comment

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

Just few minor points to clarify, other than that it looks fine!

dawn/src/dawn/Optimizer/Driver.cpp
Comment on lines +274 to +276
const IIRSerializer::Format serializationKind =
options.SerializeIIR ? IIRSerializer::parseFormatString(options.IIRFormat)
: IIRSerializer::Format::Json;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we ever used a serialization format different from json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is just defaulting to Json unless we specify the output format.

Copy link
Contributor Author

@jdahm jdahm Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe @twicki thinks we should keep the byte IO option. Thoughts @twicki, @eddie-c-davis?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree, no harm in keeping that option.

@jdahm
Copy link
Contributor Author

jdahm commented Jun 17, 2020

launch jenkins

@jdahm jdahm requested a review from Stagno June 17, 2020 23:46
@jdahm
Copy link
Contributor Author

jdahm commented Jun 17, 2020 •
edited
Loading

@Stagno Sorry for the delay! It should be ready for a final review now. Please merge overnight if you think it's ready to go!

@jdahm jdahm mentioned this pull request Jun 18, 2020
Merged
@Stagno Stagno merged commit 4c0d87c into MeteoSwiss-APN:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddie-c-davis eddie-c-davis eddie-c-davis left review comments

@Stagno Stagno Stagno approved these changes

@mroethlin mroethlin Awaiting requested review from mroethlin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdahm @Stagno @eddie-c-davis