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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

The paper link is not accessible. Could you please make it available somewhere else?

The overall approach seems very general. What's the motivation.

The current patch misses a lot the diffs. Looks like `diff 1` is missed.



================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:390
   unwrapAndPrint(OS, IR, Banner, llvm::forcePrintModuleIR());
   OS.str();
 }
----------------
Delete?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:460
 void registerChangePrinters(PassInstrumentationCallbacks &PIC) {
   if (PrintChanged) {
     static IRChangePrinter ICP(dbgs());
----------------
Use early return


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:461
   if (PrintChanged) {
     static IRChangePrinter ICP(dbgs());
     PIC.registerBeforePassCallback([](StringRef P, Any IR) {
----------------
This could be put into the lambda capture list. Maybe use shared_ptr for the IRChangePrinter instance.


================
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"),
----------------
Maybe use a pass's `PassRegistery.def` name instead of its textual class name? It would be consistent.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:235
+// 7.  When a pass is ignored (pass manager or adapter pass).
+// 8.  To compare two IR representations (of type \p T).
+template <typename T> class ChangePrinter {
----------------
Make sure we consider uselist order changes for comparison.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:632
   OptNone.registerCallbacks(PIC);
+  registerChangePrinters(PIC);
 }
----------------
For consistency, make `registerChangePrinters` a member function.


================
Comment at: llvm/test/Other/change-printer.ll:16
+; are checked.
+; N.B. As passes change functionality, this test may fail because it reports
+; whether a pass made a change or not, which can change as pass functionality
----------------
It has some non-trivial maintenance costs. I would suggest to use unit test for this patch altogether.


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

https://reviews.llvm.org/D86360