<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 <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid 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>
    <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></div></blockquote><div>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.</div><div><br></div><div>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. <a href="https://bugs.llvm.org/show_bug.cgi?id=46649" target="_blank">This</a> is the overarching "flip the NPM flag" bug, and <a href="https://bugs.llvm.org/show_bug.cgi?id=46651" target="_blank">this</a> 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 <a href="https://bugs.llvm.org/show_bug.cgi?id=46858" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=46858</a> have been investigated, I'd like to flip the flag. I'm still spending all of my time trying to get it flipped.</div><div>(some more remaining issues off the top of my head: <a href="https://bugs.llvm.org/show_bug.cgi?id=48190">coroutines don't work</a> due to the NPM CGSCC infra not properly supporting outlining, LSR doesn't preserve LCSSA in some edge cases, the AMDGPU stuff below)</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
    <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></div></blockquote><div><a href="https://bugs.llvm.org/show_bug.cgi?id=47244" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=47244</a></div><div>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.</div><div>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.</div></div></div>