[PATCH] D91890: Introduce -print-changes which show changes in patch-like format

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

There are a lot of unnecessary `llvm::`, can those be removed?

TBH this IRComparer framework seems over-engineered and it's hard for me to understand all the moving parts and how clients override certain methods, making it hard to review.
Is it possible to make it simpler?

The diff part looks mostly good though.



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:252
+// create a change reporter that uses string comparisons of the basic blocks
+// that are created using Value::print (ie, similar to dump()).
+// These classes allow one to associate extra data with each basic block
----------------
this part is unnecessary


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:277
+  // Note that the constructor will call a static function
+  // BlockDataType::initialize(BlockDataType &, BasicBlock &)
+  // to fill in the Data member.
----------------
is a separate method necessary? or can it be done in the constructor?


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:331
+  getBlockData(const StringRef S) const {
+    return (Blocks.count(S) == 1) ? &Blocks.find(S)->getValue() : nullptr;
+  }
----------------



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:348
+
+enum IRChangeDiffType { InBefore, InAfter, IsCommon };
+
----------------
can you use an `enum class`? otherwise it'll pollute the llvm namespace


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:362-363
+  // Compare the 2 IRs.  In the case of a module IR, delegate to
+  // handleFunctionCompare in the derived class for each function;
+  // otherwise delegate to handleCompare in the derived class.
+  void compare(Any IR, StringRef Prefix, StringRef PassID, StringRef Name);
----------------
These don't match the names below.
Also would it be simpler to just allow each subclass to handle Function vs Module, rather than having virtual methods for each one?


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:366
+  // Analyze \p IR and save the data in \p Data.
+  static bool analyzeIR(Any IR, IRDataType &Data);
+
----------------
the return value is never used


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:100
+static cl::opt<std::string>
+    DiffForChangeReporters("diff-for-print-changed", cl::Hidden,
+                           cl::init("/usr/bin/diff"),
----------------
`DiffBinary` or `DiffBinaryPath`?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:101
+    DiffForChangeReporters("diff-for-print-changed", cl::Hidden,
+                           cl::init("/usr/bin/diff"),
+                           cl::desc("system diff used by change reporters"));
----------------
should this default to the `diff` on the PATH?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:446
+  llvm::raw_string_ostream SS(Body);
+  B.Value::print(SS, true);
+  BlockDataType::initialize(Data, B);
----------------
does this work?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:472
+template <typename IRUnitT>
+bool IRDataTemplate<IRUnitT>::operator==(const IRDataTemplate &That) const {
+  if (llvm::StringMap<IRUnitT>::size() != That.size())
----------------
looks like `StringMap` already has an `operator==`, why recreate it here?


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

https://reviews.llvm.org/D91890