[PATCH] D91890: Introduce -print-changed=[diff | diff-quiet] which show changes in patch-like format

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

just a general comment, I do like the idea of printing diffs between passes, could make looking through pipelines much nicer



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:285
+// The data saved for comparing functions.
+class ChangedFuncData : public StringMap<ChangedBlockData> {
+public:
----------------
seems weird to inherit from StringMap, what about making it a variable instead?


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:298
+
+enum class IRChangeDiffType { InBefore, InAfter, IsCommon };
+
----------------
looks like this is only used  in the cpp file, move it there?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:107
 
+// An option for specifying the diff used by print-changed=[diff | dif-quiet]
+static cl::opt<std::string>
----------------
diff


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:110
+    DiffBinary("diff-for-print-changed", cl::Hidden, cl::init("diff"),
+               cl::desc("system diff used by change reporters"));
+
----------------
something containing the word "path" or "binary" seems clearer, like `print-changed-diff-binary` or `print-changed-diff-path`


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:125-126
+  static const unsigned NumFiles = 3;
+  static std::string FileName[NumFiles];
+  static int FD[NumFiles]{-1, -1, -1};
+  for (unsigned I = 0; I < NumFiles; ++I) {
----------------
If the temp files are removed at the end, is there any reason for these to be static? Can we just create new temp files every time?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:636
+
+const Module *ChangedIRComparer::getModule(Any IR) {
+  if (any_isa<const Module *>(IR))
----------------
I thought we already had similar utilities for this.
Should probably be a static function instead of part of ChangedIRComparer.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:641
+    const LazyCallGraph::SCC *C = any_cast<const LazyCallGraph::SCC *>(IR);
+    for (const LazyCallGraph::Node &N : *C)
+      return N.getFunction().getParent();
----------------
`C->begin()->getFunction().getParent()`. SCCs cannot be empty.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1087
+
+bool InLineChangePrinter::same(const ChangedIRData &D1,
+                               const ChangedIRData &D2) {
----------------
maybe we can just assume that `ChangedIRData` has an equality operator? not necessarily in this patch


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1103
+  const std::string NoChange = " %l\n";
+  for (const auto &B : Before) {
+    const auto &BeforeBD = B.getValue();
----------------
this won't necessarily be in the same order that the blocks are laid out in the function right? that seems like something we should make sure happens


================
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())
----------------
aeubanks wrote:
> looks like `StringMap` already has an `operator==`, why recreate it here?
ping
(and if you make ChangedFuncData contain a StringMap as a variable you can just compare the two)


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

https://reviews.llvm.org/D91890