[PATCH] D36127: [OptDiag] Updating Remarks in SampleProfile

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

In https://reviews.llvm.org/D36127#837482, @tarunr wrote:

> -pass-remarks-with-hotness doesn't seem to be helping me in getting hotness information in these remarks.


It does something (after applying your patch):

./bin/opt -sample-profile -sample-profile-file=../test/Transforms/SampleProfile/Inputs/remarks.prof -S  -pass-remarks-output=- -pass-remarks-with-hotness ../test/Transforms/SampleProfile/remarks.ll -o /dev/null | grep Hotness
Hotness:         0
Hotness:         0

but the 0 values are probably wrong.  You probably need to study/debug this.  The hotness should come from BFI which sample profiler should be updating.



================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:751-753
+        ORE.emit(OptimizationRemark(DEBUG_TYPE, "HotInline", DLoc, BB)
+                 << "inlined hot callee '" << CalledFunction->getName().str()
+                 << "' into '" << F.getName() << "'");
----------------
anemet wrote:
> Again see Inliner.cpp how to stream these out as named-value arguments.
You didn't address this.  See the Inliner or the Argument ctors; you can just stream out the Function itself.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:254
+
+  /// \brief Optimization Remark Emitter used to emit diagnostic remarks
+  OptimizationRemarkEmitter *ORE;
----------------
Comments end in period.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:730
             DI = dyn_cast<Instruction>(
-                promoteIndirectCall(I, CalledFunction, 80, 100, false)
+                promoteIndirectCall(I, CalledFunction, 80, 100, false, ORE)
                     ->stripPointerCasts());
----------------
I think that this last use of the function that didn't use to pass ORE.  Please remove the default argument from the declaration.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:750
         LocalChanged = true;
-        emitOptimizationRemark(Ctx, DEBUG_TYPE, F, DLoc,
-                               Twine("inlined hot callee '") +
-                                   CalledFunction->getName() + "' into '" +
-                                   F.getName() + "'");
+        ORE->emit(OptimizationRemark(DEBUG_TYPE, "HotInline", DLoc, BB)
+                  << "inlined hot callee '"
----------------
We can't pass I here because it's remove by the time we get here?  If yes, please comment.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1262-1264
+                  << ore::NV("Location", BranchLoc->getFilename()) << ":"
+                  << ore::NV("LineNum", BranchLoc.getLine()) << ":"
+                  << ore::NV("ColNum", BranchLoc.getCol()));
----------------
I think that the way we should handle this is like:  << NV("CondBranchesLoc", BranchLoc)

And then add a new ctor Arguments(StringRef Key, DebugLoc DL) where you set Val to DL.getFileName() + DL.getLine() + DL.getCol() and also Loc to DL.  I think this will appear correctly both on the compiler output and in YAML/HTML.

If you want to make that this works correctly in YAML/HTML, you can run opt-viewer on the YAML file.  You will have to split out the original C source from the comment into a source file specified in the debug info.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1269
+                  << " is the most popular destination for conditional branches at "
+                  << ore::NV("Unknown", StringRef("<UNKNOWN LOCATION>")));
     } else {
----------------
Just stream out a DebugLoc() in you can't find.  Don't the change the name of the field.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1449
 
-bool SampleProfileLoader::runOnModule(Module &M) {
+bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM = nullptr) {
   if (!ProfileIsValid)
----------------
Default argument on the definition?  Please remove.


Repository:
  rL LLVM

https://reviews.llvm.org/D36127