[PATCH] D29047: Introduce a basic MemorySSA updater, that supports insertDef and insertUse operations.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 5 inline comments as done.
dberlin added inline comments.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2733
+    if (auto *MUD = dyn_cast<MemoryUseOrDef>(Last))
+      BBPlace = ++(MUD->getMemoryInst()->getIterator());
+    else
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > Please remove parens
> I'm not super familiar with how BB Instruction ordering actually works, so this could totally be a misunderstanding on my part, but I think this might cause issues with operand dominance. Consider:
> 
> ```
> define void @foo() {
> entry:
>   ; 1 = MemoryDef
>   %a = call i8* @bar()
>   %b = bitcast i8* %a to i32*
>   br i1 undef label %lbb, label %lbc
> 
> lbb:
>   ; MemoryUse(1)
>   %c = load i32, i32* %b
>   br label %lbc
> 
> lbc:
>   ret void
> }
> ```
> 
> `updater.moveAfter(Use1, Def1)` will make %c dominate %b, since we call `TheLoad.moveBefore(EntryBlock, ++TheStore)`, and `++TheStore == TheBitcast`.
> 
> Same comment for blocks that only have `MemoryPhi`s, as well.
Yes, You are correct. It is possible to use moveBefore to generate illegal blocks if you are not careful.
The same is true of basicblock's movebefore.

Essentially, we do what we are told. If you tell us "put this after this def", we put it *right after this def*.

I could change it to also take a basicblock iterator if you are worried we need one.



================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2736
+      BBPlace = BB->getFirstNonPHI()->getIterator();
+    What->getMemoryInst()->moveBefore(*BB, BBPlace);
+  } else {
----------------
george.burgess.iv wrote:
> Coming into this with no updater-related context outside of this patch, it's mildly surprising to me that MemorySSA's updater moves actual `Instruction`s around in BBs. It makes sense, but can we call that we do this out in the comments, please?
We could make it not do that rather than take an iterator above?
Then the user is responsible for ordering it relative to the def.



https://reviews.llvm.org/D29047