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

Mon Aug 24 13:58:38 PDT 2020

jamieschmeiser added a comment.

Thanks for the review comments.  I have tried to address all of your concerns above and in the last set of changes posted.

================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:460
 void registerChangePrinters(PassInstrumentationCallbacks &PIC) {
   if (PrintChanged) {
     static IRChangePrinter ICP(dbgs());
----------------
ychen wrote:
> Use early return
I don't understand these comments.  All 3 callbacks are registered and there is no testing so there can be no early return.  Regarding the shared_ptr and delete comments, the object was made a member of StandardInstrumentations, which ovviates the need for the static member and control of life-span.

================
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"),
----------------
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.

================
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 {
----------------
ychen wrote:
> Make sure we consider uselist order changes for comparison.
I am not sure I understand what you mean...  If you mean that changes in order should be reported as no change, that does not happen with this option.  As the comparison exists, if the order of functions change in a module pass or the order of blocks change in a function pass, this will report it as changed as it is just comparing strings.  This may or may not be what the user desires but a more complex difference mechanism would be required to not report changes due to ordering changes only and would be a different change reporter.  It would be possible to create such a reporting system using these base classes and a subsequent change reporter that I will be putting up for review when this has landed will provide more functionality that would ease creating such a change reporter.

================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:461
+  if (PrintChanged) {
+    static IRChangePrinter ICP(dbgs());
+    PIC.registerBeforePassCallback([](StringRef P, Any IR) {
----------------
yrouban wrote:
> //static// cannot be used for a multi-threaded compiler.
> Please create //IRChangePrinter// instance for every instance of //StandardInstrumentations//.
Note that the life-span of the object extends beyond this function (which is why it was static).  I changed it to be a member of StandardInstrumentations object, which gives it the appropriate life-span and there is no longer a static member.

================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:632
   OptNone.registerCallbacks(PIC);
+  registerChangePrinters(PIC);
 }
----------------
ychen wrote:
> For consistency, make `registerChangePrinters` a member function.
Done as part of the code changes.

================
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
----------------
ychen wrote:
> It has some non-trivial maintenance costs. I would suggest to use unit test for this patch altogether.
I'm sorry but I do not understand what you mean.
I tried to choose 2 passes that currently and would likely continue to make changes to the source in the test and the tests only check that changes are made and not what the changes actually are, to avoid churn.  I think the only way to completely avoid the possibility of churn would be to not test it.

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

https://reviews.llvm.org/D86360