[PATCH] D137318: [llvm-diff] Fix false-positive diffs on forward-referencing phi nodes

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Can't say I've got quite the time to page in the algorithm, but it sounds like a reasonable idea & given this tool's probably relatively unused - I think maybe this is the right level of review & you should feel welcome to give it a go, see how it plays out in general/for your use case.

Thanks for the context & patch.



================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:184
+  // For every LBB, there may only be one corresponding RBB.
+  std::vector<BlockDiffCandidate> BlockDiffCandidates;
+  // Maps LBB to the index of its BlockDiffCandidate, if existing.
----------------
SmallVector?


================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:287-289
+      for (const auto &EquivalenceAssumption : BDC.EquivalenceAssumptions) {
+        const Value *L = EquivalenceAssumption.first;
+        const Value *R = EquivalenceAssumption.second;
----------------
Maybe?


================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:291
+        auto It = Values.find(L);
+        if ((It == Values.end() || It->second != R)) {
+          BDC.KnownToDiffer = true;
----------------



================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:348
     // FIXME: call attributes
-    if (!equivalentAsOperands(L.getCalledOperand(), R.getCalledOperand())) {
-      if (Complain) Engine.log("called functions differ");
+    AssumptionContext AC{L.getParent(), R.getParent()};
+    if (!equivalentAsOperands(L.getCalledOperand(), R.getCalledOperand(),
----------------
Could you change this and others to use something more like this/aggregate initialization (otherwise it looks like it could be a ctor call, which makes reading a bit more difficult/need more care, perhaps)


================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:551-552
+      if (!equivalentAsOperands(LO, RO, AC)) {
+        if (Complain)
+          Engine.logf("operands %l and %r differ") << LO << RO;
         return true;
----------------
would be nice to avoid these unrelated changes, if possible (could pre-commit them if you think they're valuable)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137318