[PATCH] D86360: Add new hidden option -print-changed which only reports changes to IR

[Jamie Schmeiser via Phabricator via llvm-commits]

jamieschmeiser marked an inline comment as done.
jamieschmeiser added a comment.

Thanks for doing the reviews.



================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:229
+bool isInterestingFunction(const Function &F) {
+  return !F.isDeclaration() && llvm::isFunctionInPrintList(F.getName());
+}
----------------
aeubanks wrote:
> I don't think this is necessary since the ModuleToFunctionPassAdaptor already filters declarations out.
I have removed it and marked these functions inline.  I think I put that test in while working on some of the follow-on reporters that report for modules so I suspect I will have to add it back in later.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:69
+// of this option.  Note that this option has no effect without -print-changed.
+static cl::list<std::string>
+    PrintPassesList("filter-passes", cl::value_desc("pass names"),
----------------
aeubanks wrote:
> jamieschmeiser wrote:
> > ychen wrote:
> > > Maybe use a pass's `PassRegistery.def` name instead of its textual class name? It would be consistent.
> > While it may be more consistent, I think it would also be more inconvenient for the user and would require the user to either know the PassRegistry.def id or look it up.  However, with the textual class name, it can be determined by looking at the output itself.  So, if the output is being examined and it is determined that a pass can be filtered out, the name of the pass is there in the output.  Also, to use the PassRegistry.def id would require a map to get back to the textual class name as that is what the callbacks provide and would require updating the mapping function each time a pass name changed or a new pass was added or deleted.
> I'm also in favor of using the short name like "instsimplify" that matches --print-before/after, and -passes=, it seems more consistent. If you're looking at IR, you probably are familiar with how to run passes under `opt`, which requires knowing the short name.
> 
> However, that doesn't work under the NPM yet for --print-before/after, see https://bugs.llvm.org/show_bug.cgi?id=47370.
> 
> So for now we can proceed with this, but I'll probably try and fix this up alongside --print-before/after later.
I am fine with this changing when 47370 is fixed.  I think it will be changed automatically if the pass id that is sent on the callback changes.  It will also change the name that it printed out so it will be consistent and the cut/paste aspect of my earlier comments would remain.  My main concern was having to do a mapping function that would need to be kept up to date.


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

https://reviews.llvm.org/D86360