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

Clean up the includes in the IR folder #3701

Merged
merged 9 commits into from
Dec 5, 2022
Merged

Clean up the includes in the IR folder #3701

merged 9 commits into from
Dec 5, 2022

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 16, 2022

Major rework of how the ir libraries are structured. This removes most of the includes from the ir.h file and distributes them across the compiler. There are two reasons why this is done.

  1. The way the ir includes were previously structured was very brittle. Just changing includes slightly broke the compiler. This clashed with clang-format, which tried to sort includes accordingly to the pre-defined include order. Also, the IR cpp files had to be compiled in a specific order. This makes changes or refactoring much harder than necessary.
  2. Every time ir.h was included, basically all library headers and ir headers were included. This likely causes an increase in compile time.

Now with these changes it should be much easier to make changes to the ir classes and files, including simplifying includes and dependencies.

This PR should definitely be tested on downstream extensions since it touches a core part of the compiler.

@fruffy fruffy force-pushed the fruffy/iwyu_ir branch 2 times, most recently from 2b79f7b to aa94aac Compare November 17, 2022 21:24
@fruffy fruffy force-pushed the fruffy/iwyu_ir branch 4 times, most recently from 6c292da to aa64e11 Compare November 21, 2022 00:49
@davidbolvansky davidbolvansky requested a review from mvido November 21, 2022 17:15
@fruffy fruffy force-pushed the fruffy/iwyu_ir branch 2 times, most recently from d8674e1 to befecb4 Compare November 22, 2022 00:36
frontends/p4/symbol_table.cpp Outdated Show resolved Hide resolved
frontends/parsers/v1/v1parser.ypp Outdated Show resolved Hide resolved
ir/CMakeLists.txt Outdated Show resolved Hide resolved
ir/ir.h Show resolved Hide resolved
ir/v1.def Outdated Show resolved Hide resolved
lib/source_file.h Outdated Show resolved Hide resolved
@fruffy fruffy marked this pull request as ready for review November 22, 2022 00:37
@mihaibudiu
Copy link
Contributor

You should not mix reformatting with this PR, because it makes it very difficult to review.
The ir.h including everything was by design.
You should talk to @ChrisDodd about this change.

@fruffy fruffy force-pushed the fruffy/iwyu_ir branch 6 times, most recently from 9632054 to c9fb82e Compare December 2, 2022 16:30
Copy link

@vhavel vhavel left a comment

Choose a reason for hiding this comment

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

Thanks for this, I agree it's better in long-run to maintain proper includes instead of having one "super-header". It's surprising that the amount of includes this PR needs to add is relatively low - headers are likely pulled in anyway - on the other hand it's great backends won't need many changes after this PR.

#include "frontends/common/constantFolding.h"
#include "frontends/p4/strengthReduction.h"

namespace IR {
Copy link

Choose a reason for hiding this comment

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

Headers in frontends/p4 usually put names into namespace P4, should we do the same here?

@fruffy
Copy link
Collaborator Author

fruffy commented Dec 5, 2022

Yeah, most of the back ends and the mid end just use IR nodes and visitors. The generated IR file also still pulls in a lot of includes. I think this can also be improved, but at least the file is self-contained now.

@fruffy fruffy merged commit b8823df into main Dec 5, 2022
@fruffy fruffy deleted the fruffy/iwyu_ir branch December 8, 2022 18:26
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.

3 participants