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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

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());
+  }
----------------
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.


================
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));
----------------
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.


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

https://reviews.llvm.org/D64029