[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur marked 3 inline comments as done.
xur added a comment.

I'm sorry that I missed this review for this long!

In D64029#1581952 <https://reviews.llvm.org/D64029#1581952>, @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder unittests at the moment.


I initially had the pipeline change for this. But clang did not use the default pipeline, so I removed that change.
But this makes sense now to me with a unittest.

> Probably a better option would be to define a pseudo pass name like our `default<O0>`, maybe `pgo<O0>`. Then you can parse the `O0` the same way we do for the `default` thing. when it is O0 you can call the new routine I've suggested below. When it is higher, you can call the existing routine much like the default pipeline code does. This would all be implemented inside the pass builder so no issues w/ using the utility code there.
> 
> This would let us much more nicely write tests for the pipeline fragment generated for PGO both at O0 and above -- we have tests that specifically print out the pipeline sequence. This makes it very easy to visualize changes to the pipeline. And it would let you test the O0 code path here.

hmm. I think PGO is orthogonal to default pipeline: we now have prefixes of "default", "thinlto-pre-link", "thinlto", "lto", "lto-pre-link". They all can work with PGO enabled. It seems to me PGO<O0> will not cover all of them.

> The clang part of the patch (once adapted to the narrower API) seems likely to be good then.
> 
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. Whatever is easier for you.

Great! I would prefer to have only one patch -- that would be easier.

In D64029#1581952 <https://reviews.llvm.org/D64029#1581952>, @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder unittests at the moment.
>
> Probably a better option would be to define a pseudo pass name like our `default<O0>`, maybe `pgo<O0>`. Then you can parse the `O0` the same way we do for the `default` thing. when it is O0 you can call the new routine I've suggested below. When it is higher, you can call the existing routine much like the default pipeline code does. This would all be implemented inside the pass builder so no issues w/ using the utility code there.
>
> This would let us much more nicely write tests for the pipeline fragment generated for PGO both at O0 and above -- we have tests that specifically print out the pipeline sequence. This makes it very easy to visualize changes to the pipeline. And it would let you test the O0 code path here.
>
> The clang part of the patch (once adapted to the narrower API) seems likely to be good then.
>
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. Whatever is easier for you.






================
Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+    // dramatically increase code size.
+    MPM.addPass(GlobalDCEPass());
+  }
----------------
chandlerc wrote:
> xur wrote:
> > I moved this pass under the condition when Early inline is enabled. I'm under the impression that the intention is to clean up the code for the inline.
> > Chandler: you added this pass. If you don't like this move, I can pull it out and put it under another (Level > 0) condition.
> This makes sense to me. It was probably just transcription error on my part.
Got it.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:579-602
   if (RunProfileGen) {
     MPM.addPass(PGOInstrumentationGen(IsCS));
 
-    FunctionPassManager FPM;
-    FPM.addPass(
-        createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-    MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+    if (Level > 0) {
+      FunctionPassManager FPM;
+      FPM.addPass(
+          createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
----------------
chandlerc wrote:
> Rather than moving the entire routine that is really only intended for the innards of the pass manager to be public, I'd do something a bit more narrow...
> 
> Maybe just add pipeline method to the pass builder to generate a minimal pipeline suitable for O0 usage?
> 
> I think this code will be simpler to read w/o having to have the O0 conditions plumbed all the way through it, and the duplication is tiny.
> 
> If you want, could pull out the use bits which are common and use early exit to make the code more clear:
> 
> ```
> if (!RunProfileGen) {
>   addPGOUsePasses(...);
>   return;
> }
> 
> ...
> ```
> 
> (Not sure what name to use for this, but you see the idea.) Then there would be almost no duplication between the O0 and this code, w/o having to have the conditions threaded throughout.
I added a new function named as "addPGOInstrPassesForO0" which only does the O0 passes for PGO. It is a public function as it will be called from clang.
Also followed the advice to refactor the code for better readability. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029