[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

1798 ↗

(On Diff #186036)

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 ↗

(On Diff #186297)

please also remove the declaration from MemorySSA.h

[asbirlea]

Address comment.

llvm/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.

llvm/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) &&

 #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;

llvm/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
+}
+
+