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

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus marked 5 inline comments as done.
jsilvanus added a comment.

Thanks for your review, I updated the patch to address it.

I'll wait a few days with committing so others have a chance to comment.



================
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.
----------------
dblaikie wrote:
> SmallVector?
Thanks, fixed.


================
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;
----------------
dblaikie wrote:
> Maybe?
Thanks, that's better indeed.


================
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;
----------------
dblaikie wrote:
> 
Fixed.


================
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(),
----------------
dblaikie wrote:
> 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)
Thanks, fixed.


================
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;
----------------
dblaikie wrote:
> would be nice to avoid these unrelated changes, if possible (could pre-commit them if you think they're valuable)
I removed those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137318