[RFC] Expanding the scope of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER

Project Infrastructure LLVM Dev List Archives
#1

The ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER CMake flag currently only affects Clang. It should probably also change all other uses of pass managers where possible.

There are a couple of uses inside LLD for LTO which already have new/legacy PM flags and should probably look at ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER to determine the default. Some examples.

Also at some point in the future when check-llvm has been fixed to work with opt’s -enable-new-pm flag by default, that should also be dependent upon ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER.

Any objections?

#2

https://reviews.llvm.org/D92433 for an example of changing LTO pass manager defaults (not ready to submit yet, need to fix a couple things first)

#3

I added a comment on D92433. In lld/ELF, we generally don't recommend
configure-time default values. We expect the compiler driver to handle platform differences
We expect the driver to handle platform differences (e.g. default -pie) and pass
the proper options to LLD. We can default to true and let the clang driver pass
--no-lto-new-pass-manager to LLD if needed.

#4

I added a comment on D92433. In lld/ELF, we generally don’t recommend
configure-time default values. We expect the compiler driver to handle platform differences
We expect the driver to handle platform differences (e.g. default -pie) and pass
the proper options to LLD. We can default to true and let the clang driver pass
–no-lto-new-pass-manager to LLD if needed.

That sounds fine to me as long as people don’t use lld/ELF directly. Although it can’t hurt to also change the default value,can it? :slight_smile:

For COFF/wasm does changing the default in the lld driver make sense?

#5

I added a comment on D92433. In lld/ELF, we generally don't recommend
configure-time default values. We expect the compiler driver to handle platform differences
We expect the driver to handle platform differences (e.g. default -pie) and pass
the proper options to LLD. We can default to true and let the clang driver pass
--no-lto-new-pass-manager to LLD if needed.

That sounds fine to me as long as people don't use lld/ELF directly. Although it can't hurt to also change the default value,can it? :slight_smile:

Generally lld/ELF should not be invoked directly. I expect more so for
LTO users.
I should have said that my preference to a default
--lto-new-pass-manager is weak: (a) the convention is not to have more
configure-time options
(b) having a configure-time variable for LLD does not seem to add lots
of usefulness if the compiler driver will handle it. (This will add
some complexity to tests and when it is time to move to new PM for
everything and setting will need to be changed again)

For COFF/wasm does changing the default in the lld driver make sense?

Adding Reid and Sam.

#6

I can’t speak for wasm, but on Windows people just invoke the linker directly rather than doing this strange Unix dance of calling the compiler to link. =P So, lld-link has to check the configure time setting.

#7

I strongly disagree with this proposal. As in, please do not land patches which implement this proposal. If anything, we should remove the build time config flag entirely.

The new manager is mature and has been in wide use for a long time now. Moving it to a conditional compilation item is a major regression in implied maturity and completely unwarranted. If anything, we should just flip the dang flag and make people using the old pass manager support it. (Most downstream groups I know of are running NPM.)

Philip

#8

Implementing this proposal does not in any way stop the flip of the flag, they are completely unrelated. This increases the scope of the new pass manager since much of lld’s use of LTO is currently unconditionally using the legacy PM and flipping the flag wouldn’t change that.

There are some things that the new pass manager doesn’t currently support. For example, all of AMDGPU would be broken with the switch to the new pass manager since currently AMDGPU’s passes aren’t injected into the pipeline. I’m working on the (few) remaining issues and do plan to flip the switch soon.

Also as mentioned in previous discussions, lots of people use the default, which currently is the legacy PM.

#9

You are proposing to move code for the new pass manager into conditional compilation. I am strongly opposed.

As for the overall status of the NPM, I find the continued delay in switching to be extremely problematic for the health of project long term. I understand the “X doesn’t work yet” problem, but a) X is fairly small, and b) the folks involved in maintaining X need to pay the cost of supporting the old pass manager. I do want to be careful and state explicitly that I’m expressing opinion here, not making an actual proposal. I may get around to the later eventually, but this is not it.

(two minor response inline)

Philip

“the default” for me only means opt and clang. It doesn’t mean llc, or any other tool which happens to use the old pm. If we need clang to select the old pass manager at the command line when invoking LTO, that doesn’t really bug me. I find this hard to believe. Are you possibly talking about llc/codegen? If so, that’s well out of scope for what I’m talking about. If not, can you point to a bug so I can see an example?

#10

You are proposing to move code for the new pass manager into conditional compilation. I am strongly opposed.

As for the overall status of the NPM, I find the continued delay in switching to be extremely problematic for the health of project long term. I understand the “X doesn’t work yet” problem, but a) X is fairly small, and b) the folks involved in maintaining X need to pay the cost of supporting the old pass manager. I do want to be careful and state explicitly that I’m expressing opinion here, not making an actual proposal. I may get around to the later eventually, but this is not it.

(two minor response inline)

Philip

Implementing this proposal does not in any way stop the flip of the flag, they are completely unrelated. This increases the scope of the new pass manager since much of lld’s use of LTO is currently unconditionally using the legacy PM and flipping the flag wouldn’t change that.

“the default” for me only means opt and clang. It doesn’t mean llc, or any other tool which happens to use the old pm. If we need clang to select the old pass manager at the command line when invoking LTO, that doesn’t really bug me.

This essentially proposes changing “the default” to include more. The functionality was already there, just nobody has been using it. There are no lld/LTO tests that newly fail when turning it on for the various lld LTO uses, so I don’t see the issue with this. It doesn’t make it harder to flip the flag.

I’ve been spending the last half year trying to get check-llvm green when opt is using the new pass manager by default. We’re actually very close, but not quite there. This is the overarching “flip the NPM flag” bug, and this is the bug for getting check-llvm to work with opt using the NPM. If you’d like to help with some of the last few remaining issues that’d be awesome :). Once that’s done, and some small remaining blockers like https://bugs.llvm.org/show_bug.cgi?id=46858 have been investigated, I’d like to flip the flag. I’m still spending all of my time trying to get it flipped.
(some more remaining issues off the top of my head: coroutines don’t work due to the NPM CGSCC infra not properly supporting outlining, LSR doesn’t preserve LCSSA in some edge cases, the AMDGPU stuff below)

There are some things that the new pass manager doesn’t currently support. For example, all of AMDGPU would be broken with the switch to the new pass manager since currently AMDGPU’s passes aren’t injected into the pipeline. I’m working on the (few) remaining issues and do plan to flip the switch soon.

I find this hard to believe. Are you possibly talking about llc/codegen? If so, that’s well out of scope for what I’m talking about. If not, can you point to a bug so I can see an example?

https://bugs.llvm.org/show_bug.cgi?id=47244
Backends can inject passes into the LLVM IR pipeline, for example to lower custom intrinsics. I was just about to send out a separate request to the AMDGPU community to port their stuff to the NPM, there’ll be more details there.
You can see this by setting the -enable-new-pm flag in opt to true by default, then running check-llvm to see some AMDGPU tests fail because of this missing functionality.

#11

I suspect there is a misunderstanding: the proposal is not to leave out the new PM with this flag, it is to allow the CMake flag to operate in more places:

https://github.com/llvm/llvm-project/blob/master/llvm/CMakeLists.txt#L706-L707

set(ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER FALSE CACHE BOOL
“Enable the experimental new pass manager by default.”)

This allows to build clang/LLVM with the new pass manager enabled by default. Despite the description it only affects the clang driver enabling the new PM.

The intent as I understand it is to extend the applicability of this flag, so that it applies to LTO and opt and … ; that way any buildbot with this flag turned on would check more of LLVM with the new PM.

As I understand it, this is only intended to help drive the migration further by validating the use of the new PM in more places in LLVM right now.

#12

Mehdi is completely correct. I’d assumed I knew what the flag being discussed from context did and I was wrong. Mehdi, thank you for pointing this out.

Philip

#13

For anyone reading, please disregard all of my responses to this thread (especially this one). As Mehdi was kind enough to point out downthread, I was misunderstanding the proposal from the beginning.

Arthur, my apologies for utterly derailing a conversation and for not bothering to confirm I knew what I was talking about before doing so.

Philip

#14

No problem, glad it got resolved.
And next time I should probably add more context to my llvm-dev posts.