[MemorySSA] Relax assert condition in createDefinedAccess

[StephenFan]

Summary

If globals-aa is enabled, because of the deletion of memory instructions, there
may be call instruction that is not in ModOrRefSet but is a MemoryUseOrDef.
This causes the crash in the process of cloning uses and defs.

Fixes https://github.com/llvm/llvm-project/issues/58719

[fhahn]

llvm/test/Transforms/SimpleLoopUnswitch/pr58719.ll

2

is instcombine needed here?

5

Can this load from an actual address?

[fhahn]

Thanks for the patch. it would also be great if you could mention the patch at the bug report, so people know there's a fix in the works.

[StephenFan]

1. Replace NULL as a actual address.
2. Add function(loop-mssa(loop-simplifycfg)) in passes. Only if MemorySSA is constructed before recomputing globalsaa, the crash happens.

[StephenFan]

llvm/test/Transforms/SimpleLoopUnswitch/pr58719.ll

2

Yes. InstCombine will delete the load instruction in function f.

[nikic]

I'm a bit unsure about the approach used here. Might it make more sense to relax the assertion instead? AA returning better (or worse!) results as a result of other transforms is pretty normal, so it seems fragile if code has to go out of the way to drop no longer needed MemoryAccesses to avoid assertion failures.

[StephenFan]

In D137553#3915015, @nikic wrote:

I'm a bit unsure about the approach used here. Might it make more sense to relax the assertion instead? AA returning better (or worse!) results as a result of other transforms is pretty normal, so it seems fragile if code has to go out of the way to drop no longer needed MemoryAccesses to avoid assertion failures.

To be honest, I have the same concerns. I will try to relax the assertion instead.

[StephenFan]

Relax assertion.

[nikic]

I'm fine with this change, but would be good for @asbirlea to chime in.

[asbirlea]

llvm/lib/Analysis/MemorySSA.cpp

1768 ↗

(On Diff #474173)

Could you add:

if (!Def && Use != UseCheck) {
        // New access should not have more power than template access
        assert(!UseCheck && "Invalid template");
}

[StephenFan]

Address comments.

llvm/lib/Analysis/MemorySSAUpdater.cpp

for (unsigned It = 0, E = Phi->getNumIncomingValues(); It < E; ++It) {
       }
     }
     if (auto *SingleAccess = onlySingleValue(NewPhi)) {
       MPhiMap[Phi] = SingleAccess;
       removeMemoryAccess(NewPhi);
     }
   };
 
+  auto RemoveDeadMemoryAccess = [&](BasicBlock *BB) {
+    // If globals-aa is enabled, because of the deletion of memory instructions,
+    // there may be call instruction that is not in ModOrRefSet but is a
+    // MemoryUseOrDef. This causes the crash in the process of cloning uses and
+    // defs.
+    BasicBlock *NewBlock = cast_or_null<BasicBlock>(VMap.lookup(BB));
+    if (!NewBlock)
+      return;
+    const MemorySSA::AccessList *BA = MSSA->getBlockAccesses(BB);
+    if (!BA)
+      return;
+    SmallVector<Instruction *, 2> ToRemove;
+    for (const MemoryAccess &MA : *BA) {
+      if (const MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(&MA)) {
+        Instruction *Inst = MUD->getMemoryInst();
+        if (auto *Call = dyn_cast<CallInst>(Inst)) {
+          ModRefInfo ModRef = MSSA->AA->getModRefInfo(Call, None);
+          if (!isModOrRefSet(ModRef))
+            ToRemove.push_back(Inst);
+        }
+      }
+    }
+    for (auto *Inst : ToRemove) {
+      removeMemoryAccess(Inst);
+    }
+  };
+
   auto ProcessBlock = [&](BasicBlock *BB) {
     BasicBlock *NewBlock = cast_or_null<BasicBlock>(VMap.lookup(BB));
     if (!NewBlock)
       return;
 
     assert(!MSSA->getWritableBlockAccesses(NewBlock) &&
            "Cloned block should have no accesses");
 
     // Add MemoryPhi.
     if (MemoryPhi *MPhi = MSSA->getMemoryAccess(BB)) {
       MemoryPhi *NewPhi = MSSA->createMemoryPhi(NewBlock);
       MPhiMap[MPhi] = NewPhi;
     }
     // Update Uses and Defs.
     cloneUsesAndDefs(BB, NewBlock, VMap, MPhiMap);
   };
 
   for (auto *BB : llvm::concat<BasicBlock *const>(LoopBlocks, ExitBlocks))
+    RemoveDeadMemoryAccess(BB);
+
+  for (auto *BB : llvm::concat<BasicBlock *const>(LoopBlocks, ExitBlocks))
     ProcessBlock(BB);
 
   for (auto *BB : llvm::concat<BasicBlock *const>(LoopBlocks, ExitBlocks))
     if (MemoryPhi *MPhi = MSSA->getMemoryAccess(BB))
       if (MemoryAccess *NewPhi = MPhiMap.lookup(MPhi))
         FixPhiIncomingValues(MPhi, cast<MemoryPhi>(NewPhi));
 }
 

llvm/test/Transforms/SimpleLoopUnswitch/pr58719.ll

+; RUN: opt -passes="require<globals-aa>,cgscc(instcombine),function(loop-mssa(loop-simplifycfg)),recompute-globalsaa,function(loop-mssa(simple-loop-unswitch<nontrivial>),print<memoryssa>)" -disable-output < %s 2>&1 | FileCheck %s
+
+define void @f(ptr %p) {
+entry:
+  %0 = load i16, ptr %p, align 1
+  ret void
+}
+
+define void @g(i1 %tobool.not) {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %if.then, %for.cond, %entry
+  br i1 %tobool.not, label %if.then, label %for.cond
+
+if.then:                                          ; preds = %for.cond
+; CHECK-NOT: MemoryUse(liveOnEntry)
+  call void @f()
+  br label %for.cond
+}