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

[Rong Xu via Phabricator via llvm-commits]

xur marked 7 inline comments as done.
xur 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; }
----------------
davidxl wrote:
> The comment is not precise.
Fixed


================
Comment at: lib/CodeGen/BackendUtil.cpp:690
+  }
+  if (CodeGenOpts.hasProfileCSIRInstr()) {
+    assert(!hasIRInstr && "Cannot have both IR and CSIR instrumentation");
----------------
davidxl wrote:
> what is going to happen if user uses a merged data with CS profile in profile-use and also specifies CS prof-gen?
Good question. As of this patch, the behavior is different for the legacy pass manager and the new pass manager.
For the legacy pass manager (the code here), we will call normal PGOUsePass, CSPGOGenPass, and CSPGOUsePass.
For the new pass manager, we will report error of 
Assertion `!CodeGenOpts.hasProfileCSIRUse() && "Cannot have both CSProfileUse and CSProfileGen at the same time"' failed.

The behavior for legacy pass manager is wrong. I will fix.


================
Comment at: lib/CodeGen/BackendUtil.cpp:948
+                                               : CodeGenOpts.InstrProfileOutput,
+        "", "", "", PGOOptions::IRInstr, CodeGenOpts.DebugInfoForProfiling);
+  else if (CodeGenOpts.hasProfileIRUse()) {
----------------
davidxl wrote:
> Can you explain the change at this line?
My earlier version will have two new booleans (bool RunCSProfileGen and bool RunCSProfileUse). 
Here I combined the following 3 field in PGOOptions struct
  bool RunProfileGen;
  bool RunCSProfileGen;
  bool RunCSProfileUse;
into an enum. There three are mutually exclusive.



================
Comment at: lib/Frontend/CompilerInvocation.cpp:504
+    if (PGOReader->hasCSIRLevelProfile())
+      Opts.setProfileUse(CodeGenOptions::ProfileCSIRInstr);
+    else
----------------
davidxl wrote:
> CSIRInstr and IRInstr are not exclusive to each other, so why use if-then-else here?
For CodeGenOptions kind, CSIRInstr and IRinstr are exclusive -- there is just one enum for the kind.


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

https://reviews.llvm.org/D54176