[PATCH] D158849: [GVN] Invalidate MDA when deduplicating phi nodes

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1290
+static bool
+EliminateDuplicatePHINodesSetBasedImpl(BasicBlock *BB,
+                                       SmallPtrSetImpl<PHINode *> *ToRemove) {
----------------
It might simplify the code in the  static functions  if `ToRemove` would be required for the static functions and have a loop to remove them in `EliminateDuplicatePHINodes` if the pointer is null?


================
Comment at: llvm/test/Transforms/GVN/pr64598.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S -passes=gvn < %s | FileCheck %s
+
----------------
nikic wrote:
> I got this test case by llvm-reducing between "good opt" and "bad opt", so I'm not sure how meaningful it is. I haven't pre-committed this test because address reuse might be flaky. 
> 
> I would have preferred to instead expose the issue by adding an `AssertingVH` in MemDepAnalysis. Unfortunately, the problematic value is part of `using ValueIsLoadPair = PointerIntPair<const Value *, 1, bool>` and there is no easy way to use AssertingVH in there (even if switching to std::pair, given how it is used).
I sounds like the test as is to be the best option we have for now unfortunately


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

https://reviews.llvm.org/D158849