<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>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.</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 12/4/20 4:09 PM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CANF-O=YC0r6t6LbUqfjdwjD_xa6SdHuGzDoZYtW2axKOZF8BQw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Dec 4, 2020 at
3:28 PM Philip Reames via llvm-dev <<a
href="mailto:llvm-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>You are proposing to move code for the new pass
manager into conditional compilation. I am
strongly opposed.</p>
</div>
</blockquote>
<div>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: </div>
<div><br>
</div>
<div><a
href="https://github.com/llvm/llvm-project/blob/master/llvm/CMakeLists.txt#L706-L707"
target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/blob/master/llvm/CMakeLists.txt#L706-L707</a></div>
<div>
<div>set(ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER FALSE
CACHE BOOL</div>
<div> "Enable the experimental new pass manager by
default.")</div>
</div>
<div><br>
</div>
<div>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. </div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>-- </div>
<div>Mehdi</div>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>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. <br>
</p>
<p>(two minor response inline)<br>
</p>
<p>Philip<br>
</p>
<div>On 12/4/20 3:19 PM, Arthur Eubanks wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">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.</div>
</blockquote>
"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. <br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>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.</div>
</div>
</blockquote>
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?<br>
<blockquote type="cite">
<div dir="ltr">
<div>
<div><br>
</div>
<div>Also as mentioned in previous
discussions, lots of people use the default,
which currently is the legacy PM.</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Dec 4,
2020 at 2:18 PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
target="_blank" moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>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. <br>
</p>
<p>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.)</p>
<p>Philip<br>
</p>
<div>On 12/1/20 12:34 PM, Arthur Eubanks via
llvm-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>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. <br>
</div>
<div><br>
</div>
<div>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. <a
href="https://github.com/llvm/llvm-project/blob/1314a4938fba865412598b7227cb4657d59cd8bc/lld/wasm/Driver.cpp#L382"
target="_blank"
moz-do-not-send="true">Some</a> <a
href="https://github.com/llvm/llvm-project/blob/1314a4938fba865412598b7227cb4657d59cd8bc/llvm/include/llvm/LTO/Config.h#L53"
target="_blank"
moz-do-not-send="true">examples</a>.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Any objections?</div>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
LLVM Developers mailing list
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
<a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</body>
</html>