[PATCH] D38768: Add remarks describing when a pass changes the IR instruction count of a module

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Thanks for this, I think it's going to be a useful feature (in some form), as we need to track code size more effectively.
I have some minor comments inline, and Chandler should take a look as well as he fought many battles in the PM area.



================
Comment at: lib/IR/LegacyPassManager.cpp:132
 
-
-
+int PMDataManager::getModuleInstrCount(Module &M) {
+  int Instrs = 0;
----------------
This doesn't really belong to `PMDataManager`, if we want something like this it could probably live in `Module`. 

I'm also slightly concerned as this seems to recompute the number of instruction for the whole module every time you run a function pass.
If your TU gets large (e.g. LTO) the overhead will be substantial. 
We might strive a more incremental approach, but it's not immediately clear to me how (I'll think about it)


================
Comment at: lib/IR/LegacyPassManager.cpp:148-149
+  // If it's a pass manager, don't emit a remark.
+  if (P->getAsPMDataManager())
+      return;
+
----------------
I'm not sure I understand this one, can you please give an example?


================
Comment at: lib/IR/LegacyPassManager.cpp:153
+  BasicBlock &BB = *F.begin();
+  assert(OriginalCount != FinalCount && "IR size did not change!");
+  OptimizationRemarkAnalysis R("size-info", "IRSizeChange",
----------------
we have an early return, so maybe this is redundant. I'm fine either way.


================
Comment at: lib/IR/LegacyPassManager.cpp:1553
       LocalChanged |= FP->runOnFunction(F);
+      int FinalCount = getModuleInstrCount(*F.getParent());
+      emitIRSizeChangedRemark(FP, *F.getParent(), OriginalCount, FinalCount);
----------------
`opt-remarks` is a function pass and here you're considering as unit of IR the whole module. I think the function pass manager *should* only operate on functions. This bit seems a little odd, to me.

I'd love to hear @chandlerc's thoughts on this,


https://reviews.llvm.org/D38768