[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

1770

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/MemorySSA.cpp

MemoryUseOrDef *MemorySSA::createNewAccess(Instruction *I,
   if (Template) {
     Def = isa<MemoryDef>(Template);
     Use = isa<MemoryUse>(Template);
 #if !defined(NDEBUG)
     ModRefInfo ModRef = AAP->getModRefInfo(I, None);
     bool DefCheck, UseCheck;
     DefCheck = isModSet(ModRef) || isOrdered(I);
     UseCheck = isRefSet(ModRef);
-    assert(Def == DefCheck && (Def || Use == UseCheck) && "Invalid template");
+    // Use set is not checked since AA may return better results as a result of
+    // other transforms.
+    // FIXME: Would Def value always be consistent after transforms?
+    assert(Def == DefCheck && "Invalid template");
+    if (!Def && Use != UseCheck) {
+      // New Access should not have more power than template access
+      assert(!UseCheck && "Invalid template");
+    }
 #endif
   } else {
     // Find out what affect this instruction has on memory.
     ModRefInfo ModRef = AAP->getModRefInfo(I, None);
     // The isOrdered check is used to ensure that volatiles end up as defs
     // (atomics end up as ModRef right now anyway).  Until we separate the
     // ordering chain from the memory chain, this enables people to see at least
     // some relative ordering to volatiles.  Note that getClobberingMemoryAccess

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
+
+; Check that don't crash if the Alias Analysis returns better results than
+; before when cloning loop's memoryssa.
+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
+  call void @f()
+  br label %for.cond
+}