So, I think remove from lookups should check and only remove from the lookup if the lookups point to that access<br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016, 7:23 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I have to stare at this. <br><br><div class="gmail_quote"></div><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016, 7:10 PM Sebastian Pop <<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sebpop updated this revision to Diff 66212.<br>
sebpop added a comment.<br>
<br>
Updated the patch as I first wrote it: I think this is the natural way to update an SSA variable.<br>
<br>
This fails because when invoking:<br>
+ MSSA->removeMemoryAccess(OldMemAcc);<br>
<br>
we go from:<br>
; MemoryUse(2)<br>
<br>
%2 = load i32, i32* %v.addr, align 4<br>
<br>
to no annotation at all:<br>
<br>
%2 = load i32, i32* %v.addr, align 4<br>
<br>
This is because removeMemoryAccess calls removeFromLookups which does:<br>
<br>
Value *MemoryInst;<br>
if (MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(MA)) {<br>
MemoryInst = MUD->getMemoryInst();<br>
} else {<br>
MemoryInst = MA->getBlock();<br>
}<br>
ValueToMemoryAccess.erase(MemoryInst);<br>
<br>
and erase() removes both the new and old MemoryAccess'es leaving the instruction without MSSA annotation.<br></blockquote></div><div class="gmail_quote"></div><div><br></div><div>Yes, only one memory access can exist for an instruction for real. You can't create two at once and get the right result.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For loads this can be fixed by first removing the OldMemoryAccess and then creating the NewMemoryAccess.<br>
This works for loads because there are no users of a MemoryUse.<br>
However for a store, we need to keep both the old and new MemoryAccess'es at the same time, to be able to rename the uses of old with new.<br></blockquote></div><div><br></div><div>Yes, most of the hoisters are cloning, so do not have this issue.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think the solution would be for "MSSA->removeMemoryAccess(OldMemAcc);" to only invalidate and remove "OldMemAcc" without touching "NewMemAcc" that should remain attached to the instruction.<br></blockquote></div><div><br></div><div>We can't make it work with two memoryacceses for the same instruction at once.</div><div><br></div><div>We can probably implement either a createReplacement (that takes over the uses of the old one) or a moveBefore/moveAfter.</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D22966" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22966</a><br>
<br>
Files:<br>
lib/Transforms/Scalar/GVNHoist.cpp<br>
<br>
Index: lib/Transforms/Scalar/GVNHoist.cpp<br>
===================================================================<br>
--- lib/Transforms/Scalar/GVNHoist.cpp<br>
+++ lib/Transforms/Scalar/GVNHoist.cpp<br>
@@ -193,30 +193,27 @@<br>
VN.setAliasAnalysis(AA);<br>
VN.setMemDep(MD);<br>
bool Res = false;<br>
+ MemorySSA M(F, AA, DT);<br>
+ MSSA = &M;<br>
<br>
// FIXME: use lazy evaluation of VN to avoid the fix-point computation.<br>
while (1) {<br>
- // FIXME: only compute MemorySSA once. We need to update the analysis in<br>
- // the same time as transforming the code.<br>
- MemorySSA M(F, AA, DT);<br>
- MSSA = &M;<br>
-<br>
// Perform DFS Numbering of instructions.<br>
unsigned I = 0;<br>
for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))<br>
for (auto &Inst: *BB)<br>
DFSNumber.insert({&Inst, ++I});<br>
<br>
auto HoistStat = hoistExpressions(F);<br>
- if (HoistStat.first + HoistStat.second == 0) {<br>
+ if (HoistStat.first + HoistStat.second == 0)<br>
return Res;<br>
- }<br>
- if (HoistStat.second > 0) {<br>
+<br>
+ if (HoistStat.second > 0)<br>
// To address a limitation of the current GVN, we need to rerun the<br>
- // hoisting after we hoisted loads in order to be able to hoist all<br>
- // scalars dependent on the hoisted loads. Same for stores.<br>
+ // hoisting after we hoisted loads or stores in order to be able to<br>
+ // hoist all scalars dependent on the hoisted ld/st.<br>
VN.clear();<br>
- }<br>
+<br>
Res = true;<br>
<br>
// DFS numbers change when instructions are hoisted: clear and recompute.<br>
@@ -725,6 +722,8 @@<br>
if (!Repl || firstInBB(I, Repl))<br>
Repl = I;<br>
<br>
+ MemoryAccess *NewMemAcc = nullptr;<br>
+<br>
if (Repl) {<br>
// Repl is already in HoistPt: it remains in place.<br>
assert(allOperandsAvailable(Repl, HoistPt) &&<br>
@@ -742,7 +741,23 @@<br>
!makeGepOperandsAvailable(Repl, HoistPt, InstructionsToHoist))<br>
continue;<br>
<br>
+ // Before moving the instruction, get its MSSA access.<br>
+ MemoryUseOrDef *OldMemAcc = nullptr;<br>
+ if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))<br>
+ OldMemAcc = dyn_cast<MemoryUseOrDef>(MA);<br>
+<br>
+ // Move the instruction at the end of HoistPt.<br>
Repl->moveBefore(HoistPt->getTerminator());<br>
+<br>
+ if (OldMemAcc) {<br>
+ // The definition of this ld/st will not change: ld/st hoisting is<br>
+ // legal when the ld/st is not moved past its current definition.<br>
+ MemoryAccess *Def = OldMemAcc->getDefiningAccess();<br>
+ NewMemAcc = MSSA->createMemoryAccessInBB(Repl, Def, HoistPt,<br>
+ MemorySSA::End);<br>
+ OldMemAcc->replaceAllUsesWith(NewMemAcc);<br>
+ MSSA->removeMemoryAccess(OldMemAcc);<br>
+ }<br>
}<br>
<br>
if (isa<LoadInst>(Repl))<br>
@@ -775,6 +790,14 @@<br>
} else if (isa<CallInst>(Repl)) {<br>
++NumCallsRemoved;<br>
}<br>
+<br>
+ if (NewMemAcc) {<br>
+ // Update the uses of the old MSSA access with NewMemAcc.<br>
+ MemoryAccess *OldMA = MSSA->getMemoryAccess(I);<br>
+ OldMA->replaceAllUsesWith(NewMemAcc);<br>
+ MSSA->removeMemoryAccess(OldMA);<br>
+ }<br>
+<br>
Repl->intersectOptionalDataWith(I);<br>
combineKnownMetadata(Repl, I);<br>
I->replaceAllUsesWith(Repl);<br>
<br>
<br>
</blockquote></div></blockquote></div>