[MemorySSA] Update defining access when instructions are not cloned.

[asbirlea]

Summary

Consider the case when instructions are not only simplified during
cloning (to be a use or to no longer have a MemoryAccess attached), but
they are not cloned at all, and hence not present in the map.
This is the case of debug.value calls.

Regularly we consider the new defining access as the same as the clone's
defining access, implying that, if it was not cloned, it must be outside
of the blocks being cloned.
For simplified cloning, this no longer holds. So if, an access was not
cloned, but the defining access is in the same block, look up in the Def
list to find the proper defining access.

[george.burgess.iv]

(Now that we have D67307, is this still needed? Happy to review if so, but this seems to be addressing primarily debuginfo-related bits)

[asbirlea]

I think the other patch covers this. Thanks!

lib/Analysis/MemorySSAUpdater.cpp

MPhi->unorderedDeleteIncomingIf([&](const MemoryAccess *, BasicBlock *B) {
         return true;
       Found = true;
       return false;
     });
     tryRemoveTrivialPhi(MPhi);
   }
 }
 
-static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA,
-                                                  const ValueToValueMapTy &VMap,
-                                                  PhiToDefMap &MPhiMap,
-                                                  bool CloneWasSimplified,
-                                                  MemorySSA *MSSA) {
+static MemoryAccess *
+getNewDefiningAccessForClone(MemoryAccess *MA, const ValueToValueMapTy &VMap,
+                             PhiToDefMap &MPhiMap, bool CloneWasSimplified,
+                             const MemoryUseOrDef *ClonedAccess,
   MemoryAccess *InsnDefining = MA;
   if (MemoryDef *DefMUD = dyn_cast<MemoryDef>(InsnDefining)) {
     if (!MSSA->isLiveOnEntryDef(DefMUD)) {
       Instruction *DefMUDI = DefMUD->getMemoryInst();
       assert(DefMUDI && "Found MemoryUseOrDef with no Instruction.");
       if (Instruction *NewDefMUDI =
               cast_or_null<Instruction>(VMap.lookup(DefMUDI))) {
         InsnDefining = MSSA->getMemoryAccess(NewDefMUDI);
         if (!CloneWasSimplified)
           assert(InsnDefining && "Defining instruction cannot be nullptr.");
         else if (!InsnDefining || isa<MemoryUse>(InsnDefining)) {
           // The clone was simplified, it's no longer a MemoryDef, look up.
+          // Simplified clones only occur in single block cloning from
+          // loop header/body to preheader, and DefMUD must be in the same BB
+          // as ClonedAccess, otherwise NewDefMUDI would not be in the map.
+          // So a previous definition must exist (at least a phi).
+          assert(MA->getBlock() == ClonedAccess->getBlock() &&
+                 "Defining access expected to be in the same block");
           auto DefIt = DefMUD->getDefsIterator();
-          // Since simplified clones only occur in single block cloning, a
-          // previous definition must exist, otherwise NewDefMUDI would not
-          // have been found in VMap.
           assert(DefIt != MSSA->getBlockDefs(DefMUD->getBlock())->begin() &&
                  "Previous def must exist");
           InsnDefining = getNewDefiningAccessForClone(
-              &*(--DefIt), VMap, MPhiMap, CloneWasSimplified, MSSA);
+              &*(--DefIt), VMap, MPhiMap, CloneWasSimplified, ClonedAccess,
+              MSSA);
         }
+      } else if (CloneWasSimplified &&
+                 DefMUD->getBlock() == ClonedAccess->getBlock()) {
+        // Since simplified clones only occur in single block cloning, the
+        // defining access may be MA(DefMUD), as initialized (a definition
+        // preceding the preheader into which we are cloning), but it may also
+        // be an earlier definition if an access was not cloned (debug.value
+        // calls are simplified to no-ops). Hence we need to look up.
+        auto DefIt = DefMUD->getDefsIterator();
+        assert(DefIt != MSSA->getBlockDefs(DefMUD->getBlock())->begin() &&
+               "Previous def must exist");
+        InsnDefining = getNewDefiningAccessForClone(
+            &*(--DefIt), VMap, MPhiMap, CloneWasSimplified, ClonedAccess, MSSA);
       }
     }
   } else {
     MemoryPhi *DefPhi = cast<MemoryPhi>(InsnDefining);
     if (MemoryAccess *NewDefPhi = MPhiMap.lookup(DefPhi))
       InsnDefining = NewDefPhi;
   }
   assert(InsnDefining && "Defining instruction cannot be nullptr.");
if (const MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(&MA)) {
       // Also in LoopRotate, even when it's an instruction, due to it being
       // simplified, it may be a Use rather than a Def, so we cannot use MUD as
       // template. Calls coming from updateForClonedBlockIntoPred, ensure this.
       if (Instruction *NewInsn =
               dyn_cast_or_null<Instruction>(VMap.lookup(Insn))) {
         MemoryAccess *NewUseOrDef = MSSA->createDefinedAccess(
             NewInsn,
             getNewDefiningAccessForClone(MUD->getDefiningAccess(), VMap,
-                                         MPhiMap, CloneWasSimplified, MSSA),
+                                         MPhiMap, CloneWasSimplified, MUD,
+                                         MSSA),
             /*Template=*/CloneWasSimplified ? nullptr : MUD,
             /*CreationMustSucceed=*/CloneWasSimplified ? false : true);
         if (NewUseOrDef)
           MSSA->insertIntoListsForBlock(NewUseOrDef, NewBB, MemorySSA::End);
       }
     }
   }
 }

test/Analysis/MemorySSA/debugvalue2.ll

+; RUN: opt -disable-basicaa -loop-rotate -enable-mssa-loop-dependency -verify-memoryssa -S %s | FileCheck %s
+; REQUIRES: asserts
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: @overflow_iter_var
+define void @overflow_iter_var() !dbg !11 {
+entry:
+  call void @llvm.dbg.value(metadata i16 0, metadata !16, metadata !DIExpression()), !dbg !18
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.body, %entry
+  call void @llvm.dbg.value(metadata i16 0, metadata !16, metadata !DIExpression()), !dbg !18
+  call void @llvm.dbg.value(metadata i16 undef, metadata !20, metadata !DIExpression()), !dbg !21
+  br i1 undef, label %for.end, label %for.body
+
+for.body:                                         ; preds = %for.cond
+  %0 = load i16, i16* undef, align 1
+  br label %for.cond
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3, nameTableKind: None)
+!1 = !DIFile(filename: "2_loops.c", directory: "/")
+!2 = !{}
+!3 = !{}
+!6 = !DICompositeType(tag: DW_TAG_array_type, baseType: !7, size: 4096, elements: !8)
+!7 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!8 = !{!9}
+!9 = !DISubrange(count: 256)
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = distinct !DISubprogram(name: "overflow_iter_var", scope: !1, file: !1, line: 20, type: !12, scopeLine: 21, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null, !14, !14}
+!14 = !DIBasicType(name: "unsigned int", size: 16, encoding: DW_ATE_unsigned)
+!16 = !DILocalVariable(name: "i", scope: !17, file: !1, line: 23, type: !14)
+!17 = distinct !DILexicalBlock(scope: !11, file: !1, line: 23, column: 3)
+!18 = !DILocation(line: 0, scope: !17)
+!20 = !DILocalVariable(name: "stop1", arg: 1, scope: !11, file: !1, line: 20, type: !14)
+!21 = !DILocation(line: 0, scope: !11)