<div dir="ltr">FWIW: The patch i sent should fix the issue of being able to create them. The code right now is clearly wrong, it assumes but does not check that insertion never fails, etc.<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 29, 2016 at 8:30 PM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think I have a patch that works around the limitation of only one<br>
MSSA per instruction:<br>
instead of moving the instruction that we hoist, we clone it, such<br>
that it can have its own separate MSSA annotation.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On Fri, Jul 29, 2016 at 10:23 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> Okay, so ...<br>
><br>
> you have to create before you can remove.<br>
><br>
> remove is going to destroy the memory, and not much we can do about that.<br>
> (We could split remove into remove and erase, but ... ugly)<br>
><br>
> If you create first, right now, it will do the wrong thing (the annotation<br>
> will never end up on the instruction, unless the instruction is new)<br>
> Please try the attached, which should make it stop doing the wrong thing<br>
> there. If that works, i'll add unit tests and commit it.<br>
><br>
> verifyMemorySSA will catch if you mess it up (verifyOrdering checks that the<br>
> accesslist and the lookups have the same number of things).<br>
><br>
><br>
> On Fri, Jul 29, 2016 at 7:36 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
>><br>
>> So, I think remove from lookups should check and only remove from the<br>
>> lookup if the lookups point to that access<br>
>><br>
>><br>
>> On Fri, Jul 29, 2016, 7:23 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
>>><br>
>>> I have to stare at this.<br>
>>><br>
>>> On Fri, Jul 29, 2016, 7:10 PM Sebastian Pop <<a href="mailto:sebpop@gmail.com">sebpop@gmail.com</a>> wrote:<br>
>>>><br>
>>>> 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<br>
>>>> 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<br>
>>>> instruction without MSSA annotation.<br>
>>><br>
>>><br>
>>> Yes, only one memory access can exist for an instruction for real. You<br>
>>> can't create two at once and get the right result.<br>
>>><br>
>>>><br>
>>>> For loads this can be fixed by first removing the OldMemoryAccess and<br>
>>>> 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<br>
>>>> MemoryAccess'es at the same time, to be able to rename the uses of old with<br>
>>>> new.<br>
>>><br>
>>><br>
>>> Yes, most of the hoisters are cloning, so do not have this issue.<br>
>>><br>
>>>><br>
>>>> I think the solution would be for "MSSA->removeMemoryAccess(OldMemAcc);"<br>
>>>> to only invalidate and remove "OldMemAcc" without touching "NewMemAcc" that<br>
>>>> should remain attached to the instruction.<br>
>>><br>
>>><br>
>>> We can't make it work with two memoryacceses for the same instruction at<br>
>>> once.<br>
>>><br>
>>> We can probably implement either a createReplacement (that takes over the<br>
>>> uses of the old one) or a moveBefore/moveAfter.<br>
>>>><br>
>>>><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<br>
>>>> computation.<br>
>>>> while (1) {<br>
>>>> - // FIXME: only compute MemorySSA once. We need to update the<br>
>>>> 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<br>
>>>> the<br>
>>>> - // hoisting after we hoisted loads in order to be able to hoist<br>
>>>> all<br>
>>>> - // scalars dependent on the hoisted loads. Same for stores.<br>
>>>> + // hoisting after we hoisted loads or stores in order to be<br>
>>>> 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<br>
>>>> 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,<br>
>>>> 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<br>
>>>> hoisting is<br>
>>>> + // legal when the ld/st is not moved past its current<br>
>>>> 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>
><br>
</div></div></blockquote></div><br></div>