[PATCH] D54175: [PGO] context sensitive PGO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D54175#1317243 <https://reviews.llvm.org/D54175#1317243>, @xur wrote:

> As for the tests, I meant to add test cases to this patch later, not through a different patch.


Oh got it - that's totally fine. I misunderstood.

> I can add tests now.





================
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) &&
----------------
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?


================
Comment at: lib/LTO/LTOBackend.cpp:284
   PMB.PGOSampleUse = Conf.SampleProfile;
+  PMB.EnablePGOCSInstrGen = Conf.RunCSIRInstr;
+  if (!Conf.CSIRProfile.empty()) {
----------------
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).


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

https://reviews.llvm.org/D54175