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