Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support LLVM new PassManager #8390

Merged
merged 2 commits into from
Apr 2, 2021
Merged

Conversation

mshockwave
Copy link
Contributor

Currently we are still using legacy LLVM PassManager for optimization and code generation. In this WIP draft, I'm adding an option to use the new PassManager for optimization.
There are primarily two reasons:

  1. Starting from 13.x, LLVM will be using new PassManager (for optimization) by default. This affects all the tools in the ecosystem clang, opt, and LTO. Therefore I think it's good time to kick off the migration.
  2. Theoretically, new PassManager can bring faster optimization speed. Empirical results from Clang also confirms this point.

Here is the summary and current status of the migration enclosed in this PR:

  • Add a new flag to zig: -fllvm-new-pm to switch between legacy and new PM.
  • I surprisingly found that inside a build.zig, we can't add custom compiler flag via build.Builder, so I manually add two CLI flags as the advanced options for zig build: --enable-time-report and --llvm-use-newpm for enabling time profiling and new PM, respectively. Please let me know if there is a more elegant solution.
  • Inside src/zig_llvm.cpp, add emitUsingNewPM function -- which will be enabled via the aforementioned CLI flag -- to optimize LLVM module with the new PassManager.
  • I tried to replicate the original configuration of legacy PM. But I found difficulties on two of them:
    • Optimization level. The new PM does not seem to have a separate speed and size optimization level -- you can only choose from the predefined O1~O3, Os, Oz. However, in the original config, zig setup these two levels separately.
    • Inliner. I haven't looked at the tuning option for inliner in new PM -- it doesn't expose any option on the PassBuilder interface but I'm sure there is a knob for inlining hidden somewhere.
  • We are still using legacy PM for code generation because new PM for code generation pipeline simply doesn't exist.
  • Currently this implementation is able to build stage 2 of zig. Haven't tested on other code base though.
  • Interestingly enough, the overall compilation speed of using new PM is actually 2x slower! I haven't got a chance to look deep into this issue. But on the bright side, given the fact that we need to move to new PM anyway, I'm happy that we spot this regression early and have plenty of time to address it.

Last but not the least, this PR is using LLVM release 11.x. I know some APIs and behaviors have changed on the new PM side since then, I'll see if new PM in newer version of LLVM can solve some of the problems here.

@g-w1
Copy link
Contributor

g-w1 commented Mar 29, 2021

Not sure if it was intentional to make this pr against the master branch, but zig also has a llvm12 branch that will be merged when llvm12 comes out. https://github.com/ziglang/zig/tree/llvm12

@mshockwave
Copy link
Contributor Author

mshockwave commented Mar 29, 2021

Not sure if it was intentional to make this pr against the master branch, but zig also has a llvm12 branch that will be merged when llvm12 comes out. https://github.com/ziglang/zig/tree/llvm12

Yes, I'm aware of that (and shout out to zig community for keeping tightly with LLVM releases!) and I'll probably rebase this to LLVM 12 soon. But the problem is that migrating to new PM is not an easy task -- LLVM and Clang community takes years to do that. So I thought it's a good idea to make a separate PR to keep track of it

@andrewrk
Copy link
Member

Thank you @mshockwave for taking on this project. This is some valuable work you're doing here.

I'd like to provide some guidance on a simplified approach to this problem:

First of all please do rebase these changes against the llvm12 branch. No sense in banging our heads against an old version when 12 is imminent.

Here's how I see this going:

Let's switch over to the new pass manager immediately (in the llvm12 branch). No -fllvm-new-pm switch exposed. I want to keep this to be an implementation detail. If we encounter any regressions in the test suite, or in any of our users' projects, we report the issues upstream, and revert back to legacy PM until LLVM 13. Otherwise, we're already done.


Also here is some miscellaneous replies to your bulleted points:

* enabling time profiling

Note that we already have -ftime-report which is intended to print both a timing report on zig's side of things as well as enable LLVM's time report mechanism.

* Optimization level. The new PM does not seem to have a separate speed and size optimization level -- you can only choose from the predefined O1~O3, Os, Oz. However, in the original config, zig setup these two levels separately.

The two levels are not separate on Zig's side of things either - this change actually is much closer to mapping to zig's actual optimization levels. It should be like this:

  • ReleaseFast, ReleaseSafe => O3
  • ReleaseSmall => Oz
  • Debug => Og (or O1 if Og is not available)

If you look one level up the callstack I believe you will see zig splitting its own optimization mode into separate levels; this code can be simplified to be a direct mapping instead.

* Inliner. I haven't looked at the tuning option for inliner in new PM -- it doesn't expose any option on the `PassBuilder` interface but I'm sure there is a knob for inlining hidden somewhere.

I'm guessing you're talking about the "always inliner" pass? Right now this is required to emit semantically correct code, but I do want to note that this problem will go away with self-hosted, which does forced inlining in the semantic analysis phase, and so in this case we would not need to ever enable the "always inliner" pass. However, self-hosted is not done, so we do need the "always inliner" pass enabled with the new PM.

We are still using legacy PM for code generation because new PM for code generation pipeline simply doesn't exist.

I didn't understand this point - can you elaborate?

@mshockwave
Copy link
Contributor Author

mshockwave commented Mar 29, 2021

First of all please do rebase these changes against the llvm12 branch. No sense in banging our heads against an old version when 12 is imminent.

Make sense, I'll rebase to llvm12 branch and update this PR

Here's how I see this going:

Let's switch over to the new pass manager immediately (in the llvm12 branch). No -fllvm-new-pm switch exposed. I want to keep this to be an implementation detail. If we encounter any regressions in the test suite, or in any of our users' projects, we report the issues upstream, and revert back to legacy PM until LLVM 13. Otherwise, we're already done.

Cool! I'm happy to do that

Also here is some miscellaneous replies to your bulleted points:

* enabling time profiling

Note that we already have -ftime-report which is intended to print both a timing report on zig's side of things as well as enable LLVM's time report mechanism.

Oh, I am aware of that, the new flag I added, --enable-time-report, is for zig build rather than zig. Originally I thought that in build.zig (of the zig project), I can tell build.Builder to add -ftime-report as a supplement compiler flag while building the project, but it seems that I can't do that. That's the reason I added another flag to zig build to forward the -ftime-report flag.

* Optimization level. The new PM does not seem to have a separate speed and size optimization level -- you can only choose from the predefined O1~O3, Os, Oz. However, in the original config, zig setup these two levels separately.

The two levels are not separate on Zig's side of things either - this change actually is much closer to mapping to zig's actual optimization levels. It should be like this:

  • ReleaseFast, ReleaseSafe => O3
  • ReleaseSmall => Oz
  • Debug => Og (or O1 if Og is not available)

Cool, this is also what I'm doing now

* Inliner. I haven't looked at the tuning option for inliner in new PM -- it doesn't expose any option on the `PassBuilder` interface but I'm sure there is a knob for inlining hidden somewhere.

I'm guessing you're talking about the "always inliner" pass? Right now this is required to emit semantically correct code, but I do want to note that this problem will go away with self-hosted, which does forced inlining in the semantic analysis phase, and so in this case we would not need to ever enable the "always inliner" pass. However, self-hosted is not done, so we do need the "always inliner" pass enabled with the new PM.

Make sense.

We are still using legacy PM for code generation because new PM for code generation pipeline simply doesn't exist.

I didn't understand this point - can you elaborate?

By saying code generation I'm talking about lowering LLVM IR to machine code, which is dictated by each target machine. For some reasons (for example, we don't have MachineFunction Pass in the new PM) this part has never been included in the migration plan toward new PassManager. Therefore, we can only use the new PM to optimize the LLVM IR before passing the Module instance to a separate legacy PM instance that only contains Passes for lowering to machine code.
Clang is also using this strategy right now (code).
This is, of course, not an issue for us. I just put it as a note such that people won't be confused by the fact that we still have legacy PM in our code.
(There is an ongoing efforts in the LLVM community right now to use new PM for code generation. But I guess it will take another few years 😛 )

Now zig will use new PM for optimization by default.
@mshockwave mshockwave changed the base branch from master to llvm12 March 30, 2021 01:19
@mshockwave
Copy link
Contributor Author

mshockwave commented Mar 30, 2021

Just ported the changes from LLVM 11.x to 12.x (sorry I forgot to use RC3, hope that doesn't change anything. This is the HEAD I used: e89cdf8937bb6017cc99b05823428dd2fd673368) and compare against llvm12 branch.

Everything looks fine (it can build stage2 without any problem) except that one of the tests, CBE: @setEvalBranchQuota, crashed when I run the zig build test command. I will file an issue if needed (e.g. it's not caused by new PM but LLVM 12 in general)

(NOTE: The build bots are failing because they are still configured with LLVM 11)

@andrewrk
Copy link
Member

Looks fine to me too - any reason this is still a draft?

src/zig_llvm.cpp Show resolved Hide resolved
src/zig_llvm.cpp Show resolved Hide resolved
src/zig_llvm.cpp Outdated Show resolved Hide resolved
@mshockwave
Copy link
Contributor Author

Looks fine to me too - any reason this is still a draft?

Oh okay, I'm happy to make it a normal PR

@mshockwave mshockwave marked this pull request as ready for review March 30, 2021 18:40
 - Enable MergeFunctionsPass in non-debug build.
 - Verify input and output IR when the assertion is turned on.
 - Add AlwaysInlinePass in debug build.
 - Add more comments.
@mshockwave mshockwave changed the title [WIP] Support LLVM new PassManager Support LLVM new PassManager Mar 30, 2021
Comment on lines +198 to +199
// TODO: Maybe we should collect time trace rather than using timer
// to get a more hierarchical timeline view
Copy link
Member

@andrewrk andrewrk Apr 2, 2021

Choose a reason for hiding this comment

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

can you make this a github issue instead of a TODO comment?

edit: eh it's fine. will catch it with #363 eventually.

@andrewrk
Copy link
Member

andrewrk commented Apr 2, 2021

Nice work @mshockwave - this looks ready to merge to me.

@andrewrk andrewrk merged commit 94383d1 into ziglang:llvm12 Apr 2, 2021
@andrewrk
Copy link
Member

andrewrk commented Apr 2, 2021

Please note this is reverted, see #8418 for more details. You pointed both of these issues out in this PR. Anyway now we have an issue to track it, and the next step towards landing the new PM code is to get the issues reported upstream.

@mshockwave
Copy link
Contributor Author

Please note this is reverted, see #8418 for more details. You pointed both of these issues out in this PR. Anyway now we have an issue to track it, and the next step towards landing the new PM code is to get the issues reported upstream.

Make sense, thanks for putting them into a separate issue. I'll try to sort them out and report upstream if needed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants