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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
