<div dir="ltr">they are not doing the exacly the same thing for old pass manager and new pass manger: old pass manger is doing instrumentation, while the new pass manager with this change is NOT.<div>the test is not check instrumentation, (it only check the variables that generates by InstroProfiling pass). </div><div>In this sense, the test is not well written.</div><div><br></div><div>I can draft a patch for this.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jun 28, 2019 at 4:53 PM Chandler Carruth via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</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">chandlerc added a comment.<br>
<br>
In D63155#1563229 <<a href="https://reviews.llvm.org/D63155#1563229" rel="noreferrer" target="_blank">https://reviews.llvm.org/D63155#1563229</a>>, @xur wrote:<br>
<br>
> This patch does not make sense to me.<br>
><br>
> The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.<br>
><br>
> This patch add a pass<br>
><br>
>   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));<br>
><br>
> which only gives you the wrong signal  of instrumentation is done.<br>
><br>
> I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.<br>
>  Regular FDO should not use it.<br>
><br>
> The right fix is to enable PGO instrumentation and use in pass builder for O0.<br>
><br>
> I would like to request to revert this patch.<br>
<br>
<br>
I mean, I'm happy for the patch to be reverted, but I still really don't understand why this fixes the test to work *exactly* the same as w/ the old pass manager and doesn't break any other tests if it is completely wrong? It seems like there must be something strange with the test coverage if this is so far off of correct without any failures...<br>
<br>
I also don't understand what fix you are suggesting instead, but maybe you can show a patch?<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D63155/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D63155/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D63155" rel="noreferrer" target="_blank">https://reviews.llvm.org/D63155</a><br>
<br>
<br>
<br>
</blockquote></div>