[PATCH] D36053: Refactor the build{Module|Function}SimplificationPipeline to expose optimization phase.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

I definitely lik ethis direction. Minor comments about wording / phrasing of things below.



================
Comment at: include/llvm/Passes/PassBuilder.h:74-77
+  /// \brief LLVM optimization phase.
+  ///
+  /// This enumerates the LLVM high0levle optimization phases.
+  enum OptimizationPhase {
----------------
I don't think we want this to be so open ended. Until there is some *other* necessary predicate, we should scope this very narrowly so it isn't tempting to use for other things without careful consideration.

I'd call this `ThinLTOPhase`. Also, I'd make it an enum class.


================
Comment at: include/llvm/Passes/PassBuilder.h:78-79
+  enum OptimizationPhase {
+    /// Regular optimization phase, which generated final assembly/object file.
+    Regular,
+
----------------
Based on the above, this would likely become `None` with the comment that it indicates no ThinLTO behavior needed.


================
Comment at: include/llvm/Passes/PassBuilder.h:81-90
+    /// Optimization phase invoked by ThinLTO pre-link phase.
+    /// For PGO, it invokes profile instrumentation/use at this phase.
+    /// For SamplePGO, it invokes the first profile annotation at this phase.
+    /// ICP should not be invoked in this phase.
+    PreLinkThinLTO,
+
+    /// Optimization phase invoked by ThinLTO post-link phase.
----------------
And these can just be `PreLink` and `PostLink` as they'll have a qualifier when used: `ThinLTOPhase::PreLink`.

I think the comments should also be minimal. The details of how PGO being listed here seem risky to just fall out of sync with the reality. I'd either say nothing (because `PreLink` seems pretty clear) or that they correspond to the pre-link phase of ThinLTO.


https://reviews.llvm.org/D36053