[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.

Sorry to get around to this late. Some comments. If possible, please revert and recommit the patch after we solve these remaining issues.



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:92
+// 8.  To compare two IR representations (of type \p T).
+template <typename T> class ChangePrinter {
+protected:
----------------
It would be nice to use IRUnitT instead of T.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:222
+// Return true when this is a pass for which printing of changes is desired.
+inline bool isIgnored(StringRef PassID) {
+  return PassID.startswith("PassManager<") || PassID.contains("PassAdaptor<");
----------------
There is llvm::isSpecialPass for this.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:322
+
+void handleInitialIR(Any IR, raw_ostream &Out) {
+  StringRef Banner("*** IR Dump At Start: ***");
----------------
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.


================
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:
> > 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 


================
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:
> 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.


================
Comment at: llvm/test/Other/change-printer.ll:6
+; Simple functionality check.
+; RUN: opt -S -print-changed -passes=instsimplify 2>&1 -o /dev/null < %s | FileCheck %s --check-prefix=CHECK0
+;
----------------
Instead of check0, check1, check2, could you use some short meaningful string. Say check0->check-simple, check1->check-changed-pass, etc..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86360