Philip Reames via llvm-dev
2020-Dec-04 22:18 UTC
[llvm-dev] [RFC] Expanding the scope of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER
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 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/680cf4bd/attachment.html>
Arthur Eubanks via llvm-dev
2020-Dec-04 23:19 UTC
[llvm-dev] [RFC] Expanding the scope of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER
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. 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 > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201204/2ef65e41/attachment.html>
Philip Reames via llvm-dev
2020-Dec-05 21:38 UTC
[llvm-dev] [RFC] Expanding the scope of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER
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 On 12/4/20 2:18 PM, Philip Reames 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 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/20201205/28fb8649/attachment.html>