[PATCH] D57973: [MemorySSA] Remove verifyClobberSanity.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea created this revision.
asbirlea added a reviewer: george.burgess.iv.
Herald added subscribers: Prazek, jlebar, sanjoy.
Herald added a project: LLVM.

This verification may fail after certain transformations due to
BasicAA's fragility. Added a small explanation on the method, now
unused, and a testcase that triggers the assert in checkClobberSanity.
Addresses PR40509.


Repository:
  rL LLVM

https://reviews.llvm.org/D57973

Files:
  lib/Analysis/MemorySSA.cpp
  test/Analysis/MemorySSA/pr40509.ll


Index: test/Analysis/MemorySSA/pr40509.ll
===================================================================
--- /dev/null
+++ test/Analysis/MemorySSA/pr40509.ll
@@ -0,0 +1,54 @@
+; 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 }>
+
+ at g_54 = external dso_local global i16, align 2
+ at 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
+}
+
+
Index: lib/Analysis/MemorySSA.cpp
===================================================================
--- lib/Analysis/MemorySSA.cpp
+++ lib/Analysis/MemorySSA.cpp
@@ -1778,7 +1778,6 @@
   verifyOrdering(F);
   verifyDominationNumbers(F);
   Walker->verify(this);
-  verifyClobberSanity(F);
 }
 
 /// Check sanity of the clobbering instruction for access MA.
@@ -1797,6 +1796,16 @@
 }
 
 void MemorySSA::verifyClobberSanity(const Function &F) const {
+// The original intention of this method is 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.
 #if !defined(NDEBUG) && defined(EXPENSIVE_CHECKS)
   for (const BasicBlock &BB : F) {
     const AccessList *Accesses = getBlockAccesses(&BB);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57973.186036.patch
Type: text/x-patch
Size: 3611 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190208/e9984414/attachment.bin>