[ScopInfo] Completely remove MemoryAccesses when their parent statement is removed.

Removing a statement left its MemoryAccesses in some lists and maps of
the SCoP.  Which lists depends on at which phase of the SCoP
construction the statement is deleted.  Follow-up passes could still see
the already deleted MemoryAccesses by iterating through these
lists/maps, resulting in an access violation.

When removing a ScopStmt, also remove all its MemoryAccesses by using
the same mechnism that removes a MemoryAccess.

llvm-svn: 329640

3 files changed, 135 insertions, 14 deletions

diff --git a/polly/include/polly/ScopInfo.h b/polly/include/polly/ScopInfo.h
index 5d444540e74..09bdefa208e 100644
--- a/polly/include/polly/ScopInfo.h
+++ b/polly/include/polly/ScopInfo.h
@@ -1558,7 +1558,14 @@ public:
   /// Remove @p MA from this statement.
   ///
   /// In contrast to removeMemoryAccess(), no other access will be eliminated.
-  void removeSingleMemoryAccess(MemoryAccess *MA);
+  ///
+  /// @param MA            The MemoryAccess to be removed.
+  /// @param AfterHoisting If true, also remove from data access lists.
+  ///                      These lists are filled during
+  ///                      ScopBuilder::buildAccessRelations. Therefore, if this
+  ///                      method is called before buildAccessRelations, false
+  ///                      must be passed.
+  void removeSingleMemoryAccess(MemoryAccess *MA, bool AfterHoisting = true);
 
   using iterator = MemoryAccessVec::iterator;
   using const_iterator = MemoryAccessVec::const_iterator;
@@ -2266,9 +2273,15 @@ private:
 
   /// Remove statements from the list of scop statements.
   ///
-  /// @param ShouldDelete A function that returns true if the statement passed
-  ///                     to it should be deleted.
-  void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete);
+  /// @param ShouldDelete  A function that returns true if the statement passed
+  ///                      to it should be deleted.
+  /// @param AfterHoisting If true, also remove from data access lists.
+  ///                      These lists are filled during
+  ///                      ScopBuilder::buildAccessRelations. Therefore, if this
+  ///                      method is called before buildAccessRelations, false
+  ///                      must be passed.
+  void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,
+                   bool AfterHoisting = true);
 
   /// Removes @p Stmt from the StmtMap.
   void removeFromStmtMap(ScopStmt &Stmt);
diff --git a/polly/lib/Analysis/ScopInfo.cpp b/polly/lib/Analysis/ScopInfo.cpp
index 81edb824c05..61552929f95 100644
--- a/polly/lib/Analysis/ScopInfo.cpp
+++ b/polly/lib/Analysis/ScopInfo.cpp
@@ -1819,13 +1819,15 @@ void ScopStmt::removeMemoryAccess(MemoryAccess *MA) {
   InstructionToAccess.erase(MA->getAccessInstruction());
 }
 
-void ScopStmt::removeSingleMemoryAccess(MemoryAccess *MA) {
-  auto MAIt = std::find(MemAccs.begin(), MemAccs.end(), MA);
-  assert(MAIt != MemAccs.end());
-  MemAccs.erase(MAIt);
-
-  removeAccessData(MA);
-  Parent.removeAccessData(MA);
+void ScopStmt::removeSingleMemoryAccess(MemoryAccess *MA, bool AfterHoisting) {
+  if (AfterHoisting) {
+    auto MAIt = std::find(MemAccs.begin(), MemAccs.end(), MA);
+    assert(MAIt != MemAccs.end());
+    MemAccs.erase(MAIt);
+
+    removeAccessData(MA);
+    Parent.removeAccessData(MA);
+  }
 
   auto It = InstructionToAccess.find(MA->getAccessInstruction());
   if (It != InstructionToAccess.end()) {
@@ -3553,13 +3555,19 @@ void Scop::removeFromStmtMap(ScopStmt &Stmt) {
   }
 }
 
-void Scop::removeStmts(std::function<bool(ScopStmt &)> ShouldDelete) {
+void Scop::removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,
+                       bool AfterHoisting) {
   for (auto StmtIt = Stmts.begin(), StmtEnd = Stmts.end(); StmtIt != StmtEnd;) {
     if (!ShouldDelete(*StmtIt)) {
       StmtIt++;
       continue;
     }
 
+    // Start with removing all of the statement's accesses including erasing it
+    // from all maps that are pointing to them.
+    for (auto *MA : *StmtIt)
+      StmtIt->removeSingleMemoryAccess(MA, AfterHoisting);
+
     removeFromStmtMap(*StmtIt);
     StmtIt = Stmts.erase(StmtIt);
   }
@@ -3569,7 +3577,7 @@ void Scop::removeStmtNotInDomainMap() {
   auto ShouldDelete = [this](ScopStmt &Stmt) -> bool {
     return !this->DomainMap.lookup(Stmt.getEntryBlock());
   };
-  removeStmts(ShouldDelete);
+  removeStmts(ShouldDelete, false);
 }
 
 void Scop::simplifySCoP(bool AfterHoisting) {
@@ -3592,7 +3600,7 @@ void Scop::simplifySCoP(bool AfterHoisting) {
     return RemoveStmt;
   };
 
-  removeStmts(ShouldDelete);
+  removeStmts(ShouldDelete, AfterHoisting);
 }
 
 InvariantEquivClassTy *Scop::lookupInvariantEquivClass(Value *Val) {
diff --git a/polly/test/ScopInfo/stmt_with_read_but_without_sideffect.ll b/polly/test/ScopInfo/stmt_with_read_but_without_sideffect.ll
new file mode 100644
index 00000000000..2ac6f7b61cd
--- /dev/null
+++ b/polly/test/ScopInfo/stmt_with_read_but_without_sideffect.ll
@@ -0,0 +1,100 @@
+; RUN: opt %loadPolly -polly-delicm -analyze < %s | FileCheck %s
+;
+; The statement Stmt_for_if_else_1 should be removed because it has no
+; sideeffects.  But it has a use of MemRef_tmp21 that must also be
+; removed from every list containing it.
+; This is a test-case meant for ScopInfo, but only later pass iterate
+; over the uses of MemRef_tmp21 of which the use by Stmt_for_if_else_1
+; should have been removed. We use -polly-delicm to trigger such an
+; iteration of an already deleted MemoryAccess.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+@ATH = external dso_local unnamed_addr constant [88 x float], align 16
+
+define void @setup_tone_curves() {
+entry:
+  %ath = alloca [56 x float], align 16
+  br label %for.body
+
+for.cond176.preheader:                            ; preds = %for.cond107.preheader
+  unreachable
+
+for.body:                                         ; preds = %for.cond107.preheader, %entry
+  %indvars.iv999 = phi i64 [ 0, %entry ], [ %indvars.iv.next1000, %for.cond107.preheader ]
+  %tmp5 = shl nsw i64 %indvars.iv999, 2
+  br label %for.cond7.preheader
+
+for.cond33.preheader:                             ; preds = %for.inc.1
+  br label %for.cond107.preheader
+
+for.cond7.preheader:                              ; preds = %for.inc.1, %for.body
+  %indvars.iv958 = phi i64 [ 0, %for.body ], [ %indvars.iv.next959, %for.inc.1 ]
+  %tmp20 = add nuw nsw i64 %indvars.iv958, %tmp5
+  %arrayidx = getelementptr inbounds [88 x float], [88 x float]* @ATH, i64 0, i64 %tmp20
+  %tmp21 = load float, float* %arrayidx, align 4
+  %tmp22 = add nuw nsw i64 %tmp20, 1
+  %cmp12.1 = icmp ult i64 %tmp22, 88
+  br i1 %cmp12.1, label %if.then.1, label %if.else.1
+
+for.cond107.preheader:                            ; preds = %for.cond33.preheader
+  %indvars.iv.next1000 = add nuw nsw i64 %indvars.iv999, 1
+  br i1 undef, label %for.cond176.preheader, label %for.body
+
+if.else.1:                                        ; preds = %for.cond7.preheader
+  %cmp23.1 = fcmp ogt float %tmp21, -3.000000e+01
+  br label %for.inc.1
+
+if.then.1:                                        ; preds = %for.cond7.preheader
+  %arrayidx.1 = getelementptr inbounds [88 x float], [88 x float]* @ATH, i64 0, i64 %tmp22
+  %tmp155 = load float, float* %arrayidx.1, align 4
+  %cmp16.1 = fcmp ogt float %tmp21, %tmp155
+  br label %for.inc.1
+
+for.inc.1:                                        ; preds = %if.then.1, %if.else.1
+  %min.1.1 = phi float [ %tmp155, %if.then.1 ], [ -3.000000e+01, %if.else.1 ]
+  %arrayidx.2 = getelementptr inbounds [88 x float], [88 x float]* @ATH, i64 0, i64 0
+  %tmp157 = load float, float* %arrayidx.2, align 4
+  %cmp16.2 = fcmp ogt float %min.1.1, %tmp157
+  %arrayidx.3 = getelementptr inbounds [88 x float], [88 x float]* @ATH, i64 0, i64 0
+  %tmp159 = load float, float* %arrayidx.3, align 4
+  %cmp16.3 = fcmp ogt float %tmp157, %tmp159
+  %arrayidx29 = getelementptr inbounds [56 x float], [56 x float]* %ath, i64 0, i64 %indvars.iv958
+  store float %tmp159, float* %arrayidx29, align 4
+  %indvars.iv.next959 = add nuw nsw i64 %indvars.iv958, 1
+  %exitcond961 = icmp eq i64 %indvars.iv.next959, 56
+  br i1 %exitcond961, label %for.cond33.preheader, label %for.cond7.preheader
+}
+
+
+; CHECK:      After accesses {
+; CHECK-NEXT:     Stmt_for_cond7_preheader
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_cond7_preheader[i0] -> MemRef_ATH[4p_0 + i0] };
+; CHECK-NEXT:             MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 1]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_cond7_preheader[i0] -> MemRef_tmp21[] };
+; CHECK-NEXT:            new: [p_0] -> { Stmt_for_cond7_preheader[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT:     Stmt_if_then_1
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
+; CHECK-NEXT:                 [p_0] -> { Stmt_if_then_1[i0] -> MemRef_ATH[1 + 4p_0 + i0] };
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 1]
+; CHECK-NEXT:                 [p_0] -> { Stmt_if_then_1[i0] -> MemRef_tmp21[] };
+; CHECK-NEXT:            new: [p_0] -> { Stmt_if_then_1[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT:             MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 1]
+; CHECK-NEXT:                 [p_0] -> { Stmt_if_then_1[i0] -> MemRef_min_1_1__phi[] };
+; CHECK-NEXT:            new: [p_0] -> { Stmt_if_then_1[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT:     Stmt_if_else_1_last
+; CHECK-NEXT:             MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 1]
+; CHECK-NEXT:                 [p_0] -> { Stmt_if_else_1_last[i0] -> MemRef_min_1_1__phi[] };
+; CHECK-NEXT:            new: [p_0] -> { Stmt_if_else_1_last[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT:     Stmt_for_inc_1
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 1]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_inc_1[i0] -> MemRef_min_1_1__phi[] };
+; CHECK-NEXT:            new: [p_0] -> { Stmt_for_inc_1[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_inc_1[i0] -> MemRef_ATH[0] };
+; CHECK-NEXT:             ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_inc_1[i0] -> MemRef_ATH[0] };
+; CHECK-NEXT:             MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 0]
+; CHECK-NEXT:                 [p_0] -> { Stmt_for_inc_1[i0] -> MemRef_ath[i0] };
+; CHECK-NEXT: }