[llvm-dev] [RFC] Expanding the scope of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER

Fri Dec 4 16:09:10 PST 2020

On Fri, Dec 4, 2020 at 3:28 PM Philip Reames via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> You are proposing to move code for the new pass manager into conditional
> compilation.  I am strongly opposed.
>
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.

-- 
Mehdi

> 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
> On 12/4/20 3:19 PM, Arthur Eubanks wrote:
>
> 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.
>
>
> 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?
>
>
> Also as mentioned in previous discussions, lots of people use the default,
> which currently is the legacy PM.
>
> On Fri, Dec 4, 2020 at 2:18 PM Philip Reames <listmail at philipreames.com>
> wrote:
>
>> 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
>> On 12/1/20 12:34 PM, Arthur Eubanks via llvm-dev wrote:
>>
>> 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
>> <https://github.com/llvm/llvm-project/blob/1314a4938fba865412598b7227cb4657d59cd8bc/lld/wasm/Driver.cpp#L382>
>> examples
>> <https://github.com/llvm/llvm-project/blob/1314a4938fba865412598b7227cb4657d59cd8bc/llvm/include/llvm/LTO/Config.h#L53>
>> .
>>
>> 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?
>>
>> _______________________________________________
>> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201204/bbc707fe/attachment.html>