[PATCH] D128024: [llvm-diff] Fix false alarm on PHI node

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the patch. Some comments inline



================
Comment at: llvm/test/tools/llvm-diff/phinode-1.ll:2
+; Issue: https://github.com/llvm/llvm-project/issues/56076
+; RUN: llvm-diff %s %s
+
----------------
I think this also needs a FileCheck call to check that `llvm-diff` handles the case as expected? Please see other tests like `llvm/test/tools/llvm-diff/anon-func.ll` for example.


================
Comment at: llvm/test/tools/llvm-diff/phinode-1.ll:13
+exit:
+  %res = phi double [ 0.000000e+00, %entry ], [ %0, %else ]
+  ret double 0.000000e+00
----------------
might be good to also add a test case with operands of the phi flipped.


================
Comment at: llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:594
+      return Values[L] == R || TentativeValues.count(std::make_pair(L, R)) ||
+             !diff(cast<Instruction>(L), cast<Instruction>(R), false, false);
 
----------------
Does this handle cycles in phi nodes correctly? It would be good to at least have a couple of tests with cycles.


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

https://reviews.llvm.org/D128024