[MemorySSA] Update MSSA for non-conventional AA.

[asbirlea]

Summary

Regularly when moving an instruction that may not read or write memory,
the instruction is not modelled in MSSA, so not action is necessary.
For a non-conventional AA pipeline, MSSA must be updated with the move.

[efriedma]

Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled? Would it make sense to fix that, instead of adding checks which always fail normally?

[asbirlea]

Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled?

Except for a few cases, we rely on getModRefInfo to add MemoryAccesses to Instructions, so, yes, without basicaa, we're creating accesses where we shouldn't.

Would it make sense to fix that, instead of adding checks which always fail normally?

Yes, that would make a lot of sense! I was thinking of adding this as as alternative in the patch description so I'm very happy you suggested it.

What I'm conflicted is about is how much "pruning" of things to add when MSSA is creating an access. I don't want to duplicate basicaa inside MemorySSA just because we may run without basicaa, but I also don't want to end up adding checks everywhere for the -disable-basicaa case.
As a first step I can take the conditions !Inst->mayReadFromMemory() && !Inst->mayWriteToMemory() and never create accesses for those, but I'm thinking I may still be missing cases where we're disabling basicaa and not making updates.

[efriedma]

I guess in theory it's possible, for example, for a load to be ModRefInfo::NoModRef because it loads from a readonly global, and to write a pass that depends on the assumption that we won't create a MemoryUse for that load. (I think we actually create a MemoryUse for that at the moment, but it could change.)

In practice, excluding instructions that don't access memory is probably good enough to cover all the relevant cases for passes that use MemorySSA.

[asbirlea]

Make the update in MemorySSA.

[george.burgess.iv]

thanks for this!

i like the new approach much more :)

lib/Analysis/MemorySSA.cpp

1742

does obviate the need for the recently-added isa<DbgInfoIntrinsic>()?

if so, please add a comment noting that nonstandard AA pipelines might leave us with silly modref results for I, so this check is necessary for correctness.

test/Analysis/MemorySSA/loop-rotate-disablebasicaa.ll

6

world's smallest nit: s/define void/define void/

11

should we be CHECK-NOT'ing a MemoryDef for this? seems more direct than expecting a verifier error.

[asbirlea]

Address comments.

[george.burgess.iv]

Thanks!

lib/Analysis/MemorySSA.cpp

1742

(sorry: please also remove the isa<DbgInfoIntrinsic>() above, since it's now redundant :) )

lib/Analysis/MemorySSA.cpp

MemoryUseOrDef *MemorySSA::createNewAccess(Instruction *I,
   // clobbers when we have a nonstandard AA pipeline. Ignore these fake memory
   // dependencies here.
   // FIXME: Replace this special casing with a more accurate modelling of
   // assume's control dependency.
   if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
     if (II->getIntrinsicID() == Intrinsic::assume || isa<DbgInfoIntrinsic>(II))
       return nullptr;
 
+  // Using a nonstandard AA pipelines might leave us with unexpected modref
+  // results for I, so add a check to not model instructions that may not read
+  // from or write to memory. This is necessary for correctness.
+  if (!I->mayReadFromMemory() && !I->mayWriteToMemory())
+    return nullptr;
+
   bool Def, Use;
   if (Template) {
     Def = dyn_cast_or_null<MemoryDef>(Template) != nullptr;
     Use = dyn_cast_or_null<MemoryUse>(Template) != nullptr;
 #if !defined(NDEBUG)
     ModRefInfo ModRef = AAP->getModRefInfo(I, None);
     bool DefCheck, UseCheck;
     DefCheck = isModSet(ModRef) || isOrdered(I);

test/Analysis/MemorySSA/loop-rotate-disablebasicaa.ll

+; RUN: opt -disable-basicaa -print-memoryssa -disable-output %s 2>&1 | FileCheck %s
+
+; Note: if @foo is modelled as a MemoryDef, this test will assert with -loop-rotate, due to MemorySSA not
+; being preserved when moving instructions that may not read from or write to memory.
+
+; CHECK-LABEL: @main
+; CHECK-NOT: MemoryDef
+define void @main() {
+entry:
+  br label %for.cond120
+
+for.cond120:                                      ; preds = %for.body127, %entry
+  call void @foo()
+  br i1 undef, label %for.body127, label %for.cond.cleanup126
+
+for.cond.cleanup126:                              ; preds = %for.cond120
+  unreachable
+
+for.body127:                                      ; preds = %for.cond120
+  %0 = load i16**, i16*** undef, align 1
+  br label %for.cond120
+}
+
+declare void @foo() readnone
+
+