[PATCH] D81016: Adding InlineCostAnnotationPrinterPass for Inline Cost Analysis

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

Could you split this into 2 changes:

- one that enables printing per-instruction inline cost analysis grouped together (i.e. the reason InstructionCostDetailMap is introduced)
- the other that introduces the new pass.



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:59
+    "print-inline-cost-instruction-annotations", cl::Hidden, cl::init(false),
+    cl::desc("Annotate instructions based on inline analysis"));
 
----------------
I think 'Annotate' refers to adding metadata, is that the intent? Should the rewording happen when that's implemented, rather (for clarity)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:409
+private:
+  InlineCostCallAnalyzer *ICCA;
+
----------------
Consider InlineCostCallAnalyzer *const ICCA, to force the initialization of the pointer in the ctor. This helps maintainability - we won't risk ICCA bein uninitialized; it also captures design intent - ICCA, once set, isn't re-set (IIUC).


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:413
+  InlineCostAnnotationWriter(InlineCostCallAnalyzer *ICCA) : ICCA(ICCA) {}
+  virtual void emitInstructionAnnot(const Instruction *I,
+                                    formatted_raw_ostream &OS);
----------------
drop virtual, add override, so the compiler would check for you that you're actually overriding the base class' emitInstructionAnnot, not just adding another virtual (easier to maintain code - if later there's a refactoring in the base class, for instance, it'd be caught)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:741
 
+void InlineCostAnnotationWriter::emitInstructionAnnot(
+    const Instruction *I, formatted_raw_ostream &OS) {
----------------
the move of the code in the file seems unrelated to this change - I'd propose doing it in a separate patch instead (if it's important to do).


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2524
+  TargetTransformInfo TTI(DL);
+  const InlineParams Params = llvm::getInlineParams();
+  for (BasicBlock &BB : F) {
----------------
This won't give you the Params used for inlining, it'll give you ad default Params, is that intended?


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

https://reviews.llvm.org/D81016