[PATCH] D54175: [PGO] context sensitive PGO

[Rong Xu via Phabricator via llvm-commits]

xur marked 2 inline comments as done.
xur 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:
> 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.


================
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:
> > > 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.


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

https://reviews.llvm.org/D54175