[MemorySSA] Remove verifyClobberSanity.

[asbirlea]

Summary

This verification may fail after certain transformations due to
BasicAA's fragility. Added a small explanation and a testcase that
triggers the assert in checkClobberSanity (before its removal).
Addresses PR40509.

[george.burgess.iv]

Thanks! Just a tiny nit

lib/Analysis/MemorySSA.cpp

1780–1781

If this is entirely unused now, can we stash the comments about how this isn't guaranteed in verifyMemorySSA and delete this method entirely? I'd rather not carry dead code. :)

[asbirlea]

Address comment.

[george.burgess.iv]

LGTM with one more nit. Thanks again!

lib/Analysis/MemorySSA.cpp

1799

please also remove the declaration from MemorySSA.h

[asbirlea]

Address comment.

include/llvm/Analysis/MemorySSA.h

public:
   /// Given a MemoryAccess and a Use, determine whether MemoryAccess \p A
   /// dominates Use \p B.
   bool dominates(const MemoryAccess *A, const Use &B) const;
 
   /// Verify that MemorySSA is self consistent (IE definitions dominate
   /// all uses, uses appear in the right places).  This is used by unit tests.
   void verifyMemorySSA() const;
 
-  /// Check clobber sanity for an access.
-  void checkClobberSanityAccess(const MemoryAccess *MA) const;
-
   /// Used in various insertion functions to specify whether we are talking
   /// about the beginning or end of a block.
   enum InsertionPlace { Beginning, End };
 
 protected:
   // Used by Memory SSA annotater, dumpers, and wrapper pass
   friend class MemorySSAAnnotatedWriter;
   friend class MemorySSAPrinterLegacyPass;
   friend class MemorySSAUpdater;
 
   void verifyDefUses(Function &F) const;
   void verifyDomination(Function &F) const;
   void verifyOrdering(Function &F) const;
   void verifyDominationNumbers(const Function &F) const;
-  void verifyClobberSanity(const Function &F) const;
 
   // This is used by the use optimizer and updater.
   AccessList *getWritableBlockAccesses(const BasicBlock *BB) const {
     auto It = PerBlockAccesses.find(BB);
     return It == PerBlockAccesses.end() ? nullptr : It->second.get();
   }
 
   // This is used by the use optimizer and updater.

lib/Analysis/MemorySSA.cpp

 ///
 /// \param Start     The MemoryAccess that we want to walk from.
 /// \param ClobberAt A clobber for Start.
 /// \param StartLoc  The MemoryLocation for Start.
 /// \param MSSA      The MemorySSA instance that Start and ClobberAt belong to.
 /// \param Query     The UpwardsMemoryQuery we used for our search.
 /// \param AA        The AliasAnalysis we used for our search.
 /// \param AllowImpreciseClobber Always false, unless we do relaxed verify.
-static void
+LLVM_ATTRIBUTE_UNUSED static void
 checkClobberSanity(const MemoryAccess *Start, MemoryAccess *ClobberAt,
                    const MemoryLocation &StartLoc, const MemorySSA &MSSA,
                    const UpwardsMemoryQuery &Query, AliasAnalysis &AA,
                    bool AllowImpreciseClobber = false) {
   assert(MSSA.dominates(ClobberAt, Start) && "Clobber doesn't dominate start?");
 
   if (MSSA.isLiveOnEntryDef(Start)) {
     assert(MSSA.isLiveOnEntryDef(ClobberAt) &&

 LLVM_DUMP_METHOD void MemorySSA::dump() const { print(dbgs()); }
 #endif
 
 void MemorySSA::verifyMemorySSA() const {
   verifyDefUses(F);
   verifyDomination(F);
   verifyOrdering(F);
   verifyDominationNumbers(F);
   Walker->verify(this);
-  verifyClobberSanity(F);
-}
-
-/// Check sanity of the clobbering instruction for access MA.
-void MemorySSA::checkClobberSanityAccess(const MemoryAccess *MA) const {
-  if (const auto *MUD = dyn_cast<MemoryUseOrDef>(MA)) {
-    if (!MUD->isOptimized())
-      return;
-    auto *I = MUD->getMemoryInst();
-    auto Loc = MemoryLocation::getOrNone(I);
-    if (Loc == None)
-      return;
-    auto *Clobber = MUD->getOptimized();
-    UpwardsMemoryQuery Q(I, MUD);
-    checkClobberSanity(MUD, Clobber, *Loc, *this, Q, *AA, true);
-  }
-}
-
-void MemorySSA::verifyClobberSanity(const Function &F) const {
-#if !defined(NDEBUG) && defined(EXPENSIVE_CHECKS)
-  for (const BasicBlock &BB : F) {
-    const AccessList *Accesses = getBlockAccesses(&BB);
-    if (!Accesses)
-      continue;
-    for (const MemoryAccess &MA : *Accesses)
-      checkClobberSanityAccess(&MA);
-  }
-#endif
+  // Previously, the verification used to also verify that the clobberingAccess
+  // cached by MemorySSA is the same as the clobberingAccess found at a later
+  // query to AA. This does not hold true in general due to the current fragility
+  // of BasicAA which has arbitrary caps on the things it analyzes before giving
+  // up. As a result, transformations that are correct, will lead to BasicAA
+  // returning different Alias answers before and after that transformation.
+  // Invalidating MemorySSA is not an option, as the results in BasicAA can be so
+  // random, in the worst case we'd need to rebuild MemorySSA from scratch after
+  // every transformation, which defeats the purpose of using it. For such an
+  // example, see test4 added in D51960.
 }
 
 /// Verify that all of the blocks we believe to have valid domination numbers
 /// actually have valid domination numbers.
 void MemorySSA::verifyDominationNumbers(const Function &F) const {
 #ifndef NDEBUG
   if (BlockNumberingValid.empty())
     return;

test/Analysis/MemorySSA/pr40509.ll

+; REQUIRES: asserts
+; RUN: opt -mtriple=systemz-unknown -march=z13 -O3 -enable-mssa-loop-dependency -disable-output %s
+
+; During transform to LCSSA, an access becomes obfuscated to:
+; (2 = phi (phi(val), val)), which BasicAA fails to analyze.
+; It's currently hard coded in BasicAA to return MayAlias for nested phis.
+; This leads MemorySSA to finding a new (false) clobber for a previously
+; optimized access. With verifyClobber included in verifyMemorySSA, such a
+; transformation will cause MemorySSA verification to fail.
+; If the verifyClobber is re-enabled, this test will crash.
+
+target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
+target triple = "s390x-ibm-linux"
+
+%0 = type <{ i64, i8, i64, i16 }>
+
+@g_54 = external dso_local global i16, align 2
+@g_101 = external dso_local global <{ i64, i8, i64, i8, i8 }>, align 2
+
+declare dso_local void @safe_lshift_func_int16_t_s_s()
+declare dso_local i8 @safe_div_func_int8_t_s_s()
+
+define dso_local void @func_47(%0* %arg) {
+bb:
+  %tmp = alloca i32, align 4
+  br label %bb1
+
+bb1:                                              ; preds = %bb12, %bb
+  %tmp2 = getelementptr inbounds %0, %0* %arg, i32 0, i32 3
+  store i16 undef, i16* %tmp2, align 1
+  %tmp3 = call signext i8 @safe_div_func_int8_t_s_s()
+  %tmp7 = icmp ne i8 %tmp3, 0
+  br i1 %tmp7, label %bb8, label %bb10
+
+bb8:                                              ; preds = %bb1
+  %tmp9 = icmp eq i32 0, 0
+  br i1 %tmp9, label %bb12, label %bb13
+
+bb10:                                             ; preds = %bb10, %bb1
+  call void @safe_lshift_func_int16_t_s_s()
+  %tmp11 = getelementptr inbounds %0, %0* %arg, i32 0, i32 3
+  store i16 0, i16* %tmp11, align 1
+  store i8 0, i8* getelementptr inbounds (%0, %0* bitcast (<{ i64, i8, i64, i8, i8 }>* @g_101 to %0*), i32 0, i32 1), align 2
+  br label %bb10
+
+bb12:                                             ; preds = %bb8
+  store i16 0, i16* @g_54, align 2
+  br label %bb1
+
+bb13:                                             ; preds = %bb8
+  ret void
+}
+
+