[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: include/clang/Frontend/CodeGenOptions.h:330
 
+  /// Check if IR level profile use is on.
+  bool hasProfileCSIRUse() const { return getProfileUse() == ProfileCSIRInstr; }
----------------
The comment is not precise.


================
Comment at: lib/CodeGen/BackendUtil.cpp:690
+  }
+  if (CodeGenOpts.hasProfileCSIRInstr()) {
+    assert(!hasIRInstr && "Cannot have both IR and CSIR instrumentation");
----------------
what is going to happen if user uses a merged data with CS profile in profile-use and also specifies CS prof-gen?


================
Comment at: lib/CodeGen/BackendUtil.cpp:948
+                                               : CodeGenOpts.InstrProfileOutput,
+        "", "", "", PGOOptions::IRInstr, CodeGenOpts.DebugInfoForProfiling);
+  else if (CodeGenOpts.hasProfileIRUse()) {
----------------
Can you explain the change at this line?


================
Comment at: lib/CodeGen/BackendUtil.cpp:954
     PGOOpt = PGOOptions("", CodeGenOpts.ProfileInstrumentUsePath, "",
-                        CodeGenOpts.ProfileRemappingFile, false,
+                        CodeGenOpts.ProfileRemappingFile, Action,
                         CodeGenOpts.DebugInfoForProfiling);
----------------
Explain the change at this line.


================
Comment at: lib/CodeGen/BackendUtil.cpp:967
+  if (CodeGenOpts.hasProfileCSIRInstr()) {
+    assert(!CodeGenOpts.hasProfileCSIRUse() &&
+           "Cannot have both CSProfileUse and CSProfileGen at the same time");
----------------
should emit error message instead.


================
Comment at: lib/CodeGen/BackendUtil.cpp:1244
+  // Context sensitive profile.
+  if (CGOpts.hasProfileCSIRInstr()) {
+    Conf.RunCSIRInstr = true;
----------------
please add +tejohnson for reviewing thinlto related pieces.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:504
+    if (PGOReader->hasCSIRLevelProfile())
+      Opts.setProfileUse(CodeGenOptions::ProfileCSIRInstr);
+    else
----------------
CSIRInstr and IRInstr are not exclusive to each other, so why use if-then-else here?


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

https://reviews.llvm.org/D54176