[PATCH] D54175: [PGO] context sensitive PGO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/Passes/PassBuilder.h:44
         SamplePGOSupport(SamplePGOSupport || !SampleProfileFile.empty()) {
-    assert((RunProfileGen ||
-            !SampleProfileFile.empty() ||
-            !ProfileUseFile.empty() ||
-            SamplePGOSupport) && "Illegal PGOOptions.");
+    assert((Action != NoAction || !SampleProfileFile.empty() ||
+            !ProfileUseFile.empty() || SamplePGOSupport) &&
----------------
tejohnson wrote:
> xur wrote:
> > tejohnson wrote:
> > > Shouldn't this be (Action == NoAction || ...)? I.e. either we are doing no PGO action, or we better have one of the other fields set?
> > There are 4 possible values for Action: NoAction, IRInstr, CSIRInstr, CSIRUse
> > If NoAction, we need to have SampleProfileFile  nonempty or ProfileUseFile nonempty. They are for sample fdo and fdo-use.
> > If the value is of the other 3 values, we don't check SampleProfile and ProfileUseFile.
> > 
> > I think this is correct but I agree the check can be improved.
> I see. I think it would be a lot clearer if the Action was extended then to cover all the possible pgo types, it's confusing to have NoAction be set when e.g. sample profile or IR profile use is enabled. 
Please address this comment (it's really unintuitive to have NoAction mean sample profile or IR profile use).


================
Comment at: lib/LTO/LTOBackend.cpp:284
   PMB.PGOSampleUse = Conf.SampleProfile;
+  PMB.EnablePGOCSInstrGen = Conf.RunCSIRInstr;
+  if (!Conf.CSIRProfile.empty()) {
----------------
tejohnson wrote:
> xur wrote:
> > tejohnson wrote:
> > > xur wrote:
> > > > tejohnson wrote:
> > > > > Is it deliberate that here in the old PM the RunCSIRInstr and CSIRProfile are handled regardless of whether SampleProfile is set, whereas in the new PM they are only handled when SampleProfile is not set? What should be the behavior if sample profile is specified along with CS profiling?
> > > > It's not deliberate. I should have make the behavior consistent. 
> > > > I think I would like to change new PM and make CSProfile independent of SampleProfile.
> > > > The reason is that -fprofile-use and -fprofile-sample-use are independent. 
> > > > This usage does not make a lot of sense, and we should probably give some warnings. But that should be in command line handling, not here.
> > > > 
> > > > What do you think?
> > > Yeah it would be good to make them consistent. Looking at the new PM though, I'm not sure how you can make them independent with the existing PGOOptions interface.
> > > 
> > > I noticed a similar inconsistency between new and old PM on the clang side patch, although it looks like it  predates your changes (with the newPM the sample profile is only looked at if no IR profiling enabled, in the old PM it is looked at unconditionally).
> > SampleProfileFile, ProfileGenFile and ProfileUseFile are of different fields in PGOOptions interface. I think technically they can be independently set /checked. But it's really confusing if SampleProifle and Profile{Gen|Use}File are both on.
> Agree. Probably add some checking into PGOOptions that only one is set (maybe in combination with the extension to Action that I suggested above)?
ping on this suggestion.


================
Comment at: lib/Passes/PassBuilder.cpp:768
+  // sensitive PGO pass.
+  if (!LTOPreLink && PGOOpt &&
+      (PGOOpt->Action == PGOOptions::CSIRInstr ||
----------------
Add comment about the LTOPreLink check.


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:566
+  if (!(PrepareForLTO || PrepareForThinLTO))
+    addPGOInstrPasses(MPM, true);
+
----------------
Document constant parameter (here and elsewhere). E.g. /*IsCS=*/true.


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

https://reviews.llvm.org/D54175