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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:322
+
+void handleInitialIR(Any IR, raw_ostream &Out) {
+  StringRef Banner("*** IR Dump At Start: ***");
----------------
jamieschmeiser wrote:
> ychen wrote:
> > Why is the round-trip of defining these here and passing them into the ctor of IRChangePrinter? Why do we need private callbacks such as ChangePrinter::HandleInitialIR. It seems just a trivial wrapper of handleInitialIR.
> The reason for the round-trip structure is that there are more instantiations of these templates that report changes in different forms still to come.  I will be subsequently posting reviews for at least 2 different change reporters based on these base classes.  By defining these templates in this fashion, the functionality of comparing different IR representations and determining when changes have occurred and should be filtered, reported or ignored is all in common in the base classes.  The derived classes will be called when interesting events happen and only need to handle presenting the events without having to determine when the events occur.
Maybe I missed something. This sounds like either virtual inheritance or CRTP are better alternatives than many function objects as class members. I would prefer CRTP.


================
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 {
----------------
jamieschmeiser wrote:
> ychen wrote:
> > ychen wrote:
> > > jamieschmeiser wrote:
> > > > 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.
> > > Uselist order 
> > (Maybe as a followup work) Please check the declaration of `Module::print`. Makes sure *ShouldPreserveUseListOrder* is true when printing IR for comparison. It should work out-of-box for Module, but needs some work for other IR units. This is helpful for tracking passes that may transform IR differently depending on the uselist order.
> I'm sorry but I still do not understand...  This unordered_set is just a list of pass names that are specified using the hidden option -filter-passes to allow filtering of output for passes.  So, if you do not want output for a particular pass, you specify it with -filter-passes and that pass will be listed as being filtered out even if it makes changes.  This is similar to -filter-print-funcs and this function is similar to llvm::isFuntionInPrintList.  The output for a module is generated using unwrapAndPrint so I think the change that you are implying would be made in printIR that takes a Module (near line 139) and would also affect other users of that code (such as print-[before|after]-all).  I think such a change should be handled separately as it doesn't just affect this new code and I am not sure of the implications.
Sorry. The position of this comment is a bit misplaced. What I meant is that uselist order is also part of IR (https://llvm.org/docs/LangRef.html#use-list-order-directives) but it is not printed by default. When we comparing IR here, it would be helpful to make sure uselist order changes are also captured by this patch.


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

https://reviews.llvm.org/D86360